Chore: Make installing the pre-commit hook "required"#2295
Conversation
|
rwgk
left a comment
rwgk
left a comment
There was a problem hiding this comment.
Nice!
Did you already look into running a one-time backfill, to make all copyright years correct based on git history? I think otherwise merge commits might have spurious year updates for a while, until we organically got everything.
I did this when I first introduced the copyright year pre-commit check. But it's possible some errors have been introduced since. |
| To set yourself up for running pre-commit checks locally and to catch issues before pushing your changes, follow these steps: | ||
|
|
||
| * Install pre-commit with: `pip install pre-commit` | ||
| * Run this once per checkout: `pre-commit install` |
There was a problem hiding this comment.
Non-blocking: I thought pre-commit -a below would take care of this step?
There was a problem hiding this comment.
It only takes care of it if you remember to run it before every single commit.
The driver for this change is that some people forget to run it with every commit (easy to do), and CI only checks the last commit. Most of the time, that's equivalent, but with copyright year handling (which needs to know what changed) and .pyi handling (which can introduce conflicts) it isn't equivalent.
|
I think requiring pre-commit to be installed and run is a no-brainer. But isn't it a tautology that we add a pre-commit check to see if pre-commit is installed? If pre-commit is not run and files drift, wouldn't our pre-commit.ci catch it and fail? Not sure how this PR would help (apart from the doc change)... |
There is, unfortunately, no way to insist that everyone has it installed before making a commit. This PR helps for the case of people who have been running If the config change seems too niche, we could just merge the documentation (policy) change here instead. |
It'd be my preference. Sometimes I do web-based development through GitHub UI, and there is no way for me to run pre-commit locally. As long as pre-commit.ci still runs and catches issues, I can just leave a comment and let it fix for me. |
To be clear, this doesn't affect that workflow at all. There's no way to truly enforce -- this is just a nudge. The pre-commit CI won't ever give the warning. |
That can be "too late", unless we can teach pre-commit.ci to run individually for each commit that's on top of I think this is a useful PR, at least for our team. I'd expect that everybody runs pre-commit at least sometimes, in most environments. That's when the nudge will fire. If pre-commit isn't available, there will be no nudge.
Not in this PR for sure, but a belts-and-suspenders workflow that ensures the copyright years are accurate, and ideally also checks the generated .pyi stubs, would be great. |
4 participants
I think we should start encouraging /all/ developers to install the pre-commit hook, not just running it manually. I expect some controversy on this one.
Even though we do "squash on merge" and having the last commit in a PR is usually good enough, the copyright year updater can not handle this correctly (since it has no way of otherwise knowing when a file was updated). The
.pyigenerator can create merge conflicts (in the ctk-next workflow) if it is not always self-consistent with every commit.I think we should stay true to the phrase "pre-commit" and ensure this is run "pre- every single commit".