-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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
CI/STYLE: run black before ruff in pre-commit #51416
CI/STYLE: run black before ruff in pre-commit #51416
Conversation
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.
good one, thanks
The right order is actually ruff- black - ruff (optional) (see astral-sh/ruff#3143) Although it seems we're just days/weeks away from |
I don't think we already let ruff autofix things, in which case that shouldn't be needed to run black again afterward? But yes, good to know (we probably also want to start using autofix at some point) |
the issue with autofixing is that due to pandas semantics it doesn't work particularly well with E711 pandas/pandas/tests/groupby/test_groupby_dropna.py Lines 653 to 654 in d8a1a0d
Maybe we can just turn E711 off and let it autofix though |
Just wanted to correct this real quick for anybody else who ends up here through a search engine: the right order is actually Ruff -> Black -> Ruff (optional). As stated in the linked issue:
Originally posted by @charliermarsh in astral-sh/ruff#3143 (comment) And also as exemplified in their pre-commit config: https://github.com/astral-sh/ruff/blob/3ccd1d580d2d680ad6d4bb7faec16bca39af53f7/.pre-commit-config.yaml#L45-L61 |
Thanks for correcting that @cosmojg ! I've also updated my original comment (for the same reason -- in case someone ends up here via a search engine) |
As I mentioned before, as far as I understand, running ruff before black is only important if you let ruff autofix things? (or are there other reasons to run it before black?) And we don't do that (yet) in pandas. In that case (not needing the initial ruff to autofix), the "ruff - black - ruff" sequence becomes "black - ruff", which is what we do. |
@jorisvandenbossche Correct, running Ruff before Black is only important if you let Ruff autofix things. To clarify, I was only trying to correct @timvink's comment as this thread appears second on the results page when searching for "pre-commit black before ruff or ruff before black" on Kagi. Sorry about the notification! And thank you for all your hard work on Pandas! |
@jorisvandenbossche No problem, thanks for the clarification! |
Something I have been noticing recently: when commit / running pre-commit, I would often get some warnings from ruff, but for things that black will fix afterwards anyway. While there is no harm done here, it can still be confusing, thinking you also need to fix those warnings. So it might be better to first run black and then ruff.
This commit does that, moving black first. Because it is a "local" hook, we now have two "local" sections, but that seems to work fine.