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

pre-commit tool setup issue (pip) #11754

Open
c-taylor opened this issue Sep 4, 2024 · 2 comments
Open

pre-commit tool setup issue (pip) #11754

c-taylor opened this issue Sep 4, 2024 · 2 comments

Comments

@c-taylor
Copy link

c-taylor commented Sep 4, 2024

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

  • 'Modern' brew python does not have 'pip' only 'pip3'
  • virtualenv and other yapf tools scripts hard require pip
  • Scripts use python3 by name, but not pip3 (seems inconsistent)
  • repo yapf is from 2021

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
Copy link
Contributor

bneradt commented Sep 4, 2024

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
Copy link
Contributor

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

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

No branches or pull requests

3 participants