kinoute
commented
1 year ago Interested by this as well.
Open applied-mathematician opened 1 year ago
kinoute
commented
1 year ago Interested by this as well.
Alex-ley-scrub
commented
2 months ago @charliermarsh I know you said this on the isort meta issue https://github.com/astral-sh/ruff/issues/6190#issuecomment-1664881638:
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 https://github.com/astral-sh/ruff/labels/good%20first%20issue - 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
commented
1 month ago 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:
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-L76annotate_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// 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-L255crates/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
commented
1 month ago Thanks @Alex-ley-scrub for your interest.
@charliermarsh I know you said this on the isort meta issue https://github.com/astral-sh/ruff/issues/6190#issuecomment-1664881638:
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
The
isortsettingsimport_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.