Skip to content

Conversation

@eddiebergman
Copy link
Collaborator

Checks everyday to create PR that is to update pre-commit hooks

See #1153

@PGijsbers
Copy link
Collaborator

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

on:
  release:
    types:
      - published

should do the trick. What do you think?

@eddiebergman
Copy link
Collaborator Author

eddiebergman commented Jul 11, 2022

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

@PGijsbers
Copy link
Collaborator

you may want to have the updates as part of the release

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.

So this should only make changes to the pre-commit config file which shouldn't cause merge conflicts

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.

@eddiebergman
Copy link
Collaborator Author

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,
Eddie

@PGijsbers
Copy link
Collaborator

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 pre-commit run --all-files before pushing the changes for a PR would likely solve that issue (provided CI reports the mypy problems).

@eddiebergman
Copy link
Collaborator Author

That's also a great point, I'll add those as extra steps :)

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

Successfully merging this pull request may close these issues.

2 participants