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

STY: switching from black to ruff-format ? #4748

Closed
neutrinoceros opened this issue Nov 22, 2023 · 7 comments · Fixed by #4868
Closed

STY: switching from black to ruff-format ? #4748

neutrinoceros opened this issue Nov 22, 2023 · 7 comments · Fixed by #4868
Labels
code style Related to linting tools proposal Proposals for enhancements, changes, etc
Milestone

Comments

@neutrinoceros
Copy link
Member

ruff now comes with a super fast formatter, which draws heavily from black. The formatter is currently in beta but is expected to reach maturity very soon.
There are a couple intentional deviations from black's style, which currently affect about 100 lines of code in yt (roughly 0.1%), and to me they all make sense.

Do we want to migrate from black to ruff at some point ?

Expected gains:

  • 3x faster runs for running pre-commit eagerly (pre-commit run --all-files) locally as well as pre-commit.ci (from 20s to 7s as of my latest measurement)
  • one less pre-commit hook to keep updated and manage

Predictable costs:

To me gains seem to outweigh costs, so I'd propose to perform the migration sometimes after the formatter is out of beta, if a consensus is reached.

@neutrinoceros neutrinoceros added proposal Proposals for enhancements, changes, etc code style Related to linting tools labels Nov 22, 2023
@tusharsadhwani
Copy link

blacken-docs-like support is apparently planned to be added to ruff very soon. heard on twitter.

@neutrinoceros
Copy link
Member Author

Yes, there are a number of actively discussed tickets about it, most prominently astral-sh/ruff#7146

@neutrinoceros
Copy link
Member Author

black 24.1.0 is here, and should land in a pre-commit.ci auto PR about 10 days from now.
I ran some experiments to measure the impact of different scenarios. I'm using git diff main --stat

  • black 23 ➡️ black 24 : 52 files changed, 92 insertions(+), 89 deletions(-)
  • black 23 ➡️ black 24 ➡️ ruff-format : 57 files changed, 147 insertions(+), 119 deletions(-)
  • black 23 ➡️ ruff-format: 27 files changed, 102 insertions(+), 87 deletions(-)

In my opinion, smaller diffs are preferable, but the comparison might be slightly biased towards ruff-format, which is intended to follow black in its yearly style update but hasn't done it yet, so its current style is intentionally closer to black 23 at the moment. Easiest course of action is probably to let pre-commit upgrade to black 24 for now, and maybe move to ruff-format once it's marked as stable.

@matthewturk
Copy link
Member

OK, this is pretty convincing. I think it might be time!

@neutrinoceros
Copy link
Member Author

Currently there's no replacement for blacken-docs so we'd have to manage 2 very-slightly different styles and hooks if we migrated now

I'm not sure when it happened but now ruff-format has native support for formatting docs, so I don't think this is a limitation any more.

@matthewturk
Copy link
Member

Then shall we?

@neutrinoceros
Copy link
Member Author

Yes, but just to avoid shooting ourselves in the foot by making some backports harder, I suggest we do that after yt 4.3.1 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code style Related to linting tools proposal Proposals for enhancements, changes, etc
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants