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

CI/STYLE: run black before ruff in pre-commit #51416

Merged

Conversation

jorisvandenbossche
Copy link
Member

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.

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

good one, thanks

@MarcoGorelli MarcoGorelli added this to the 2.0 milestone Feb 15, 2023
@MarcoGorelli MarcoGorelli added the Code Style Code style, linting, code_checks label Feb 15, 2023
@jorisvandenbossche jorisvandenbossche merged commit 58cc356 into pandas-dev:main Feb 16, 2023
@jorisvandenbossche jorisvandenbossche deleted the pre-commit-back-first branch February 16, 2023 07:28
@timvink
Copy link

timvink commented Feb 23, 2023

The right order is actually ruff- black - ruff (optional) (see astral-sh/ruff#3143)

Although it seems we're just days/weeks away from ruff being able to blacken the code for you also (astral-sh/ruff#1904)

@jorisvandenbossche
Copy link
Member Author

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)

@MarcoGorelli
Copy link
Member

the issue with autofixing is that due to pandas semantics it doesn't work particularly well with E711

# flake8 doesn't like the vectorized check for None, thinks we should use `is`
| ((values == None) & ((values == None).cumsum() <= 5)) # noqa: E711

Maybe we can just turn E711 off and let it autofix though

@cosmojg
Copy link

cosmojg commented Oct 2, 2023

The right order is actually black - ruff - black (see astral-sh/ruff#3143)

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:

          (We test in CI that running Ruff, then Black, then Ruff again is a stable operation.)

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

@timvink
Copy link

timvink commented Oct 3, 2023

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)

@jorisvandenbossche
Copy link
Member Author

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.

@cosmojg
Copy link

cosmojg commented Oct 12, 2023

@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
Copy link
Member Author

@jorisvandenbossche No problem, thanks for the clarification!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Style Code style, linting, code_checks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants