Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support heading setting for isort #6371

Open
Tracked by #6190
applied-mathematician opened this issue Aug 6, 2023 · 4 comments
Open
Tracked by #6190

Support heading setting for isort #6371

applied-mathematician opened this issue Aug 6, 2023 · 4 comments
Labels
isort Related to import sorting needs-decision Awaiting a decision from a maintainer

Comments

@applied-mathematician
Copy link

applied-mathematician commented Aug 6, 2023

The isort settings import_heading_{section_name} lets you specify heading comments for the sections, each defaulting to None.

I use these headings with all of my team projects so not a second is wasted trying to figure out where a package comes from. This is especially helpful when you have only a couple imports, hence not all sections. Developers new to a team have to figure out whether a package name they don't recognize is internal or third-party. There are even standard libraries that experienced developers often don't know, e.g. ast. This time savings translates to tasks like skimming your modules to gather a precise list of third-party dependencies.

@charliermarsh charliermarsh added the isort Related to import sorting label Aug 6, 2023
@kinoute
Copy link

kinoute commented Sep 11, 2023

Interested by this as well.

@Alex-ley-scrub
Copy link

@charliermarsh I know you said this on the isort meta issue #6190 (comment):

I'm open to contributions for additional isort options, but we likely won't prioritize implementing them ourselves

I'd personally love this isort heading setting to be implemented since it would allow me to migrate us over to ruff isort from our current isort configuration. We use it for the same reason as @applied-mathematician

I saw that this one was not labelled as a good first issue Good for newcomers - I was trying to gauge how difficult it would be to implement. I've been learning Rust off and on for the last 2 years and I've written some with pyO3 to speed up our python codebase but I'm definitely not (yet) as proficient as I am in other languages.

Knowing at present very little about the ruff codebase and the isort implementation makes it difficult to estimate the time/effort needed and wether this would be a good candidate for my first contribution or not.

I've had a read of CONTRIBUTING.md and I've had a look around the isort implementation folder. I am guessing that this a point at which the insertion of the import section header (comment) could naively occur ruff_linter/src/rules/isort/mod.rs#L201-L208. i.e. we could just insert the import section header when we are adding a newline before the section (as per the code comment // Add a blank line between every section) but of course that doesn't handle a lot of edge cases like if the import section header was already present, how do we detect that and how does this import section header interact with other comment lines present and other potentially conflicting settings such as no-lines-before etc.

Would it be possible for you to outline how you think it could/should be implemented when you have time please? And if I think I can do it then I'll give it a go, if not perhaps someone else who reads your outline will feel confident to do so - or I can come back to it at a later date.

@Alex-ley-scrub
Copy link

Hey @charliermarsh / @MichaReiser - I've recently been committing to https://github.com/getgrit/gritql/commits?author=Alex-ley-scrub and getting familiar with another AST / parsing heavy rust repo. It is really nice to write rust in a more mature and structured project (as opposed to just writing hotpath python stuff in rust with pyO3).

I think I'm ready to give this isort header issue an attempt now. I'm going to start with this approach unless you guys have some better ideas:

  1. add a new field to the Settings struct - i.e. import_heading: FxHashMap<ImportSection, String> or BTreeMap<ImportSection, String> for the isort section here: https://github.com/astral-sh/ruff/blob/main/crates/ruff_linter/src/rules/isort/settings.rs#L47-L76
  2. inside of annotate_imports() and/or normalize_imports() remove any exact matches for header strings that were defined in import_heading (as we will add them back later in the correct places): https://github.com/astral-sh/ruff/blob/main/crates/ruff_linter/src/rules/isort/mod.rs#L81-L90 or just filter them out of ImportCommentSet.atop and ImportFromCommentSet.atop inside of categorize_imports() etc. https://github.com/astral-sh/ruff/blob/main/crates/ruff_linter/src/rules/isort/mod.rs#L172
  3. try the naive insertion mentioned in my last message https://github.com/astral-sh/ruff/blob/main/crates/ruff_linter/src/rules/isort/mod.rs#L201-L208 // Add a blank line between every section but handling correctly the no_lines_before setting etc. - I guess I could do 2 and 3 inside of format::format_import() / format::format_import_from() potentially: https://github.com/astral-sh/ruff/blob/main/crates/ruff_linter/src/rules/isort/mod.rs#L229-L255
  4. write a bunch of tests and try to think of all the edge case and adapt the code as necessary (i.e. add fixtures in crates/ruff_linter/resources/test/fixtures/isort/ and add those to the test functions such as no_sections() and add new test functions as required to test new settings combinations)

#[derive(Debug, Clone, CacheKey)]
#[allow(clippy::struct_excessive_bools)]
pub struct Settings {
    ...
    pub no_lines_before: FxHashSet<ImportSection>,
    pub import_heading: FxHashMap<ImportSection, String>,
    ...
}

usage:

[tool.ruff.lint.isort]
force-sort-within-sections = false
required-imports = ["from __future__ import annotations"]
# https://github.com/astral-sh/ruff/issues/6371
import-heading.future = "Future imports (must occur at the beginning of the file):"
import-heading.standard-library = "Standard library imports:"
import-heading.third-party = "Third party imports:"
import-heading.first-party = "Local imports:"
extra-standard-library = ["pathlib", "packaging", "glob", "urllib"]
known-first-party = ["src"]
known-third-party = ["wandb", "pandas", "numpy"]
no-lines-before = ["local-folder"]
section-order = [
    "future",
    "standard-library",
    "third-party",
    "first-party",
    "local-folder",
]

@MichaReiser
Copy link
Member

Thanks @Alex-ley-scrub for your interest.

@charliermarsh I know you said this on the isort meta issue #6190 (comment):

I'm open to contributions for additional isort options, but we likely won't prioritize implementing them ourselves

We've since changed our stand and are less likely to accept new isort settings. I'm not familiar with isort myself so I'm unlikely the right person to make this decision but I would wait until we hear from @charliermarsh

@MichaReiser MichaReiser added the needs-decision Awaiting a decision from a maintainer label Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
isort Related to import sorting needs-decision Awaiting a decision from a maintainer
Projects
None yet
Development

No branches or pull requests

5 participants