-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
chore: set up ruff as a new linter/formatter #28158
Conversation
Wait what, black is no longer the new black? I took a look at ruff, pretty great perf and overall promises. But let's test this carefully, as even many mature linters fall apart on the Superset codebase.. |
It seems like perf is a big sell,
however I think Black only takes a few seconds to runs and thus any speed up is irrelevant from a UX perspective in my mind. I've never once thought "you know the main problem with Black is that it's too slow". @mistercrunch would you mind including some links as to why we should be adopting this over Black et al.? Sure some of the auto-formatting looks great—including being more opinionated about blank lines—however on the flip side the one import per line (with the addition of the |
Hey I should have written more originally when submitting this draft, but this is meant as an experiment around whether it would make sense to add this package as part of the stack. I'll try and summarize my findings and recommendation after playing with it. Early impression are: on the positive:
on the negative:
Overall I think the recommendation is likely to be "we should do this now" or "we should probably revisit in a little while " |
98b7877
to
33175b0
Compare
@villebro @john-bodley - my recommendation is to move forward with this, speed and centralization of the tooling make things really nice. Big wins include:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to @mistercrunch comments on upgrading to this package.
@john-bodley for formatting >99.9% compatible to black reading the release statements
Optimized for adoption. Ruff’s formatter is designed as a drop-in replacement for Black. On Black-formatted Python projects, Ruff achieves >99.9% compatibility with Black, as measured by changed lines. When formatting the Django codebase, for example, Ruff and Black only differ on 34 out of 2,772 files. Focusing on compatibility enables projects to adopt the Ruff formatter with minimal disruption.
https://astral.sh/blog/the-ruff-formatter#
This one will conflict with everything (touching 574 files), I'd love to merge quickly(!) |
Great resources here too -> https://docs.astral.sh/ruff/faq/ For those interested in the story behind the project -> https://www.youtube.com/watch?v=LCva0NOM2-o |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do this!
@mistercrunch did you try out |
@andy-clapson first I hear of it, but OMG if this delivers on its promises THE FUTURE IS HERE. Oh is it the same dude!? Yes! We depend on |
For the record, I opened an issue asking about our use cases -> astral-sh/uv#5487 |
Yeah, we started using it a few months back and it's extremely fast. |
SUMMARY
Ruff is great, and it's fast. All the cool kids are using it.
#edit -putting my latest comment as PR body:
This PR is meant as an experiment around whether it would make sense to add this package as part of the stack. I'll try and summarize my findings and recommendation after playing with it.
relevant: https://docs.astral.sh/ruff/faq/
Early impression are:
on the positive:
ruff
(pylint uses in its repo for instance). There's fair amount of cognitive load in dealing with disjointed tools with their own cli, config and quirks.--add-no-qa
--ignore-noqa
and--fix
on the negative:
Overall I think the recommendation is likely to be "we should do this now" or "we should probably revisit in a little while", will update here once I form an opinion.