Skip to content

Set up Ruff to lint imports#733

Closed
AA-Turner wants to merge 4 commits intopypa:mainfrom
AA-Turner:sort-imports
Closed

Set up Ruff to lint imports#733
AA-Turner wants to merge 4 commits intopypa:mainfrom
AA-Turner:sort-imports

Conversation

@AA-Turner
Copy link
Contributor

@AA-Turner AA-Turner commented Mar 3, 2025

From #732, I personally find it useful to have a tool check these things automatically for me.

This PR proposes a minimal Ruff configuration, setting up only unused import detection (F401) and import sorting. I've also taken the liberty to propose a GitHub workflow to check this.

However, I saw in @cdce8p's #726 that you "don't generally favour large-scale code cleanups or strict enforcement of style rules", so I won't be saddened if this PR gets rejected -- see it as just a suggestion of what might be useful!

A


N.B. There are several options to configure import sorting, e.g. force-sort-within-sections. I'm not sure if you have pre-existing preferences, so I just went with the isort defaults to begin with.

Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about this one. I do prefer the imports to be sorted. However, I'm not sure ruff should be used for that. The select config does have the unfortunate consequence that more and more rules will be enabled as time goes one even if they might not be particularly relevant or even desirable.

Maybe a better approach would be to use isort as it's much more limited in scope? I'd also suggest to only add the pre-commit config for it.

If at some point, these should be enforced via CI, we could use https://pre-commit.ci for that.

--
One a related note, I'd personally also be open to adding a code formatter (preferably black) in a separate PR. Have to admit, when those were first added to one of the projects I work on, I was quite skeptical. However, over time it has grown on me and most could I write myself nowadays already follows these guidelines anyway.

@AA-Turner
Copy link
Contributor Author

However, I'm not sure ruff should be used for that. The select config does have the unfortunate consequence that more and more rules will be enabled as time goes one even if they might not be particularly relevant or even desirable.

Could you elaborate? An explicit select as in this PR limits to just the isort rules and F401, nothing else will be added in updates of stuff unless explicitly opted in to. The benefit of using ruff over isort is that it is drastically faster, completing in less time than Python takes to start up!

A

@cdce8p
Copy link
Member

cdce8p commented Mar 4, 2025

Could you elaborate? An explicit select as in this PR limits to just the isort rules and F401, nothing else will be added in updates of stuff unless explicitly opted in to.

Of course. Was more thinking about what will happen in the future. I've seen it on multiple project now. First someone adds a limited set of carefully selected ruff rules. After some time, you'll get some random "drive-by" contributor who sees that and thinks they can help contribute to a project by enabling additional rules. It just tends to inflate a lot with time, unfortunately.

The benefit of using ruff over isort is that it is drastically faster, completing in less time than Python takes to start up!

Yes, though isort should still be fast enough here. Especially as pre-commit isn't required. So enabling it is fully optional.

@takluyver
Copy link
Member

Thanks for the interest, but I am going to decline this for precisely the reason you mentioned - I personally prefer less tooling and less rigid rules about code style, and I don't like reformatting PRs changing lots of code.

If I do get persuaded on something like sorting imports, I also have a rather untrendy preference for Python developer tools that are written in Python - like isort, unlike ruff - so we can readily understand and modify them without having to learn Rust.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants