Skip to content

Conversation

@cdeil
Copy link
Contributor

@cdeil cdeil commented Feb 2, 2025

This PR has three commits that are best reviewed one by one:

  • 5f03082 improves make.py a bit
  • 226e4e5 has some ruff format changes on arcade (not many changes, and very little ones removing empty lines or adding trailing commas mostly)
  • c1ac696 switches make.py, pyproject.toml and CONTRIBUTING.md from black to ruff format

Related: #2139, #2165, #2214

Looks like .pre-commit-config.yaml already only runs ruff format, no black.
So overall I think this is almost a drop-on replacement, just with some small gains in simplicity / consistency / speed.

Thoughts?

@einarf
Copy link
Member

einarf commented Feb 2, 2025

@cdeil
Copy link
Contributor Author

cdeil commented Feb 10, 2025

Yes? No? Maybe?

@einarf
Copy link
Member

einarf commented Feb 10, 2025

Probably. No resistance doing this change but properly mapping out the differences and double checking the config options is a good idea. Also need to make sure the tooling is up to date for the environments people are currently using.

Resolved a conflict with dev branch to keep this up to date.

@cdeil
Copy link
Contributor Author

cdeil commented Feb 10, 2025

There's infos on ruff format here: https://docs.astral.sh/ruff/formatter/

Basically it's 99.9% compatible with Black:

As such, the formatter is designed as a drop-in replacement for Black, but with an excessive focus on performance and direct integration with Ruff. Given Black's popularity within the Python ecosystem, targeting Black compatibility ensures that formatter adoption is minimally disruptive for the vast majority of projects.

The 0.1% deviations are explained here: https://docs.astral.sh/ruff/formatter/black/

I think you're already running ruff format and not black currently on precommit:

# Run the formatter.
- id: ruff-format

So this PR would align devdocs / CI in line and go all-in on ruff format vs the current solution of using two formatters and be faster. I think that's the main/only advantage, I would be surprised if anyone cares about the tiny formatting differences between the two.

Let me know if there's anything else I can do here.

@einarf
Copy link
Member

einarf commented Feb 10, 2025

Yup. We seems to have the majority of the maintains on board. I will adapt one of my local projects quickly first before I merging this just to get the full picture even if it's meant to be a drop-in replacement.

Will merge in the next few days.

The pre-commit I think was only a test that @eruvanos was using. It had some challenges depending on how your environment is set up. We should probably look into that as well soon.

@pushfoo
Copy link
Member

pushfoo commented Feb 10, 2025

I'm up for merging this.

  1. You fixed some weird fstring issues
  2. The root cause for them still isn't fixed in black Combine f-strings which would be placed on the same line psf/black#4389

@pushfoo
Copy link
Member

pushfoo commented Feb 17, 2025

#2569 just got hit with the exact issue I mentioned (f-string handling in psf/black#4389).

I will adapt one of my local projects quickly first before I merging this just to get the full picture even if it's meant to be a drop-in replacement.

@einarf any reasons not to merge this?

@einarf
Copy link
Member

einarf commented Feb 21, 2025

#2569 just got hit with the exact issue I mentioned (f-string handling in psf/black#4389).

I will adapt one of my local projects quickly first before I merging this just to get the full picture even if it's meant to be a drop-in replacement.

@einarf any reasons not to merge this?

Be free to merge. I have been swamped with work.

Copy link
Member

@pushfoo pushfoo left a comment

Choose a reason for hiding this comment

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

TL;DR: Let's do it. 🚀

I have one minor personal style preference to gripe over, and that's handled by # fmt: off + # fmt: on if I really need it.

@pushfoo pushfoo merged commit 79defbf into pythonarcade:development Feb 23, 2025
8 checks passed
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