pythonarcade / arcade

Easy to use Python library for creating 2D arcade games.
http://arcade.academy
Other
1.66k stars 313 forks source link

Autoformatting #2139

Open benjamin-kirkbride opened 1 week ago

benjamin-kirkbride commented 1 week ago

How do people feel about implementing (and enforcing) autoformatting in Arcade? Specifically:

Some considerations:

Cleptomania commented 1 week ago

As far as ruff goes, we already use it for linting, so using it for the import sorting is trivial.

Really just need to exclude the examples code from autoformatting as far as exclusions go, maybe the experimental directory because there are technically examples in that? Not sure autoformatting matters for any of those though.

I don't think we care about git blame preservation

einarf commented 1 week ago

We definitely don't care about blame preservation.

Cleptomania commented 1 week ago

Things that probably need to happen for this:

  1. A single, giant PR for initial conversion
  2. Added to the CI to check for formatting(probably not having CI actually do any formatting though)
  3. Add a tool to make.py to run it(CI could just use this probably)
  4. Update contributing docs for it
benjamin-kirkbride commented 1 week ago

Isn't step one actually some kind of broader consensus? Who needs to sign off?

benjamin-kirkbride commented 1 week ago

Autoformatting the imports may break a bunch of stuff at first, due to.. load bearing import order

Cleptomania commented 1 week ago

Those steps were just like, assuming it was happening, we probably have very little load bearing import order left. The 3.0 work cleaned up a lot of Arcade import. I would probably be in favor of excluding __init__.py files from the formatting though, the top level one is probably a minefield

eruvanos commented 1 week ago

gui is already formated with ruff and I use pre-commit hooks to enforce it on my side.

pushfoo commented 1 week ago

TL;DR: I am very wary of this idea.

The most important part of this (imo) is knowing how to turn it off. The following probably need to be excluded:

We're refactoring the utils folder one step at a time (#2109), but it's probably best to leave it alone for now.

As to other auto-formatting options not mentioned above, there are good reasons to disable them in many cases. PEP-8 itself points out that following it dogmatically is bad. I don't think you're proposing we do so, but I'm bringing it up now so we can agree not to do so in the future.

einarf commented 6 days ago

All done

benjamin-kirkbride commented 5 days ago

Did we sort imports too as part of this? Sorry if I missed that

Cleptomania commented 5 days ago

2141 just covered implementing black. Intended to follow up with import sorting separately, I’ll just reopen this for that.

einarf commented 5 days ago

.. or just make separate issue.