-
-
Notifications
You must be signed in to change notification settings - Fork 211
Add: workflow to update precommit #1154
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
Conversation
|
Thanks, does this also automatically add the reformatted code to the PR (in case black's formatting rules changed in the update)? It's not clear from the little documentation of the action. Second, I think checking for pre-commit updates daily might be a bit disruptive and modifying the workflow to trigger on release makes more sense. This mostly avoids situations where we can have merge conflicts because a feature branch was in development while the same code was reformatted in the development branch (as hopefully most feature branches are merged before release). I think should do the trick. What do you think? |
|
So this should only make changes to the pre-commit config file which shouldn't cause merge conflicts, i.e. a PR made from the workflow can safely be merged without disrupting another feature branch. The conflict would only occur if someone in their feature branch decides to change the pre-commit config file manually. With respect to releases, it does make sense but you may want to have the updates as part of the release, rather than only realizing it after the release has been done. However I'm happy to change this to whatever you think makes the most sense |
Which they will be... the next release. These dependencies are only for developers and govern the code formatting/guidelines and generally have no effect on the functionality of the tool.
Only updating the pre-commit file without updating the affected files may lead to a state where CI will do a pre-commit check to make sure everything is formatted/type-hinted correctly and fail. This can happen when e.g., black's formatting rules update and would reformat some files. That said, it shouldn't be hard to update the workflow to add that step to the PR. |
|
Yup, your points makes sense, I'll change it later today. Regarding updates causing format conflicts, most common precommit hooks that would format code are fairly stable, i.e. their updates are primarily new functionality, covering new features, optimizations and weird edge cases. I've had to update mypy once for it to recognize valid typing I wrote but that's the only time an updated pre-commit made a meaningful difference for me. Best, |
|
Our last bump changed 50 files (#1150). Granted, we had not updated the dependencies since we added the pre-commit hooks, but that was "just" under 2 years ago. I do concede that the times the dependencies actually change formatting will not be common (most edits will be caused by one or two specific version bumps). Auto-updating the pre-commit file only without showing the affected files (if any) does not seem all that useful to me since I wouldn't do a blind merge for the above reasons and thus still need to figure out which (if any) files are affected manually. But adding a step in the job with |
|
That's also a great point, I'll add those as extra steps :) |
10d7f17 to
f37ebbe
Compare
Checks everyday to create PR that is to update pre-commit hooks
See #1153