apache / trafficserver

Apache Traffic Serverâ„¢ is a fast, scalable and extensible HTTP/1.1 and HTTP/2 compliant caching proxy server.
https://trafficserver.apache.org/
Apache License 2.0
1.81k stars 804 forks source link

pre-commit tool setup issue (pip) #11754

Open c-taylor opened 1 month ago

c-taylor commented 1 month ago

I encountered a few issues when setting up the tools for pre-commit hooks on a recent brew:

Additionally: pip installs make me uncomfortable. Maybe consider using system yapf/clang-format and publishing config rather than version locking?

Reproduction: Remove all of your yapf/clang tooling in your repo Try to recreate it

bneradt commented 1 month ago

Thanks for the observations.

Maybe consider using system yapf/clang-format and publishing config rather than version locking?

We version lock these formatters to ensure that they format identically across all users. Different versions may behave differently with different configs. If something even very small changes about the way a tool formats, it gets in the way of a developer committing a patch because the different CI format will block it and there won't be much the dev can do. Even if they manually edit to satisfy CI, their own pre-commit hook will block them. So they will have to remove their pre-commit hook, which will then miss other formatting issues (maybe with clang-format) that won't get caught until CI runs.

Having said that, we can look into updating yapf. I switched to yapf from autopep8 due to bugs in the recent version of autopep8 - with Python 3.12, autopep8 was simply broken. I actually contributed observations from ATS to the bug here. If I recall correctly (it's been about 10 months, so my recollection is hazy) I chose the latest version of yapf at the time that could easily be used across the Python versions we support. But maybe things are different now.

bryancall commented 1 month ago

@c-taylor Has this been fixed for you? If so, can you please close the issue.