Add tool to keep test-requirements in sync with pre-commit #3234
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3234 +/- ##
====================================================
- Coverage 100.00000% 99.93727% -0.06274%
====================================================
Files 124 126 +2
Lines 19047 19129 +82
Branches 1287 1296 +9
====================================================
+ Hits 19047 19117 +70
- Misses 0 10 +10
- Partials 0 2 +2
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Thanks this makes sense and seems pretty simple. We could probably make it simpler too.
I wonder whether maybe this should update the pre-commit configuration too, to maximize the version (lexicographically? Or just import packaging) between both files.
Another helpful related script might be to copy the pins from the requirements.txt files to the pre-commit file.
| changed = False | ||
| old_lines = requirements.read_text(encoding="utf-8").splitlines(True) | ||
|
|
||
| with requirements.open("w", encoding="utf-8") as file: |
There was a problem hiding this comment.
I kind of feel like a loop over the version_data dict with a regex replacement might be simpler. Though this is already plenty simple.
| # Get tool versions from pre-commit | ||
| # Get correct names | ||
| pre_commit_versions = { | ||
| name.removesuffix("-mirror").removesuffix("-pre-commit"): version |
There was a problem hiding this comment.
IMO we should just do a dict of url to package name at the top instead of trying to automatically detect it
There was a problem hiding this comment.
I was intending for this to be general enough to work even if new hooks are added in the future, but if necessary I could change it to be a static dictionary
There was a problem hiding this comment.
I don't think this looks very general (what if the name ends with -precommit? or similar) and it's not an error if something that could be picked up automatically isn't, so I think I'd prefer a manual addition.
I don't know of anything I would add that this wouldn't work with, but I also don't know anything I would add to the pre-commit configuration to begin with!
| print("Changed test requirements version to match pre-commit") | ||
| print(f"{name}=={old_version} -> {name}=={version}") | ||
| file.write(new_line) | ||
| return changed |
There was a problem hiding this comment.
This should also check whether every entry in version_data was used
There was a problem hiding this comment.
I don't immediately understand why this should be the case. For example, typos exists only as a pre-commit hook, and in fact I don't know if it even exists as a python package. Similar with zizmor.
There was a problem hiding this comment.
Yeah this would only make sense if #3234 (comment) was implemented.
While this could be useful in some cases, I don't see a need for that in a regular use case. From my point of view, most of the time someone (or pre-commit.ci) would run The primary use case for this script is so that when pre-commit ci automatically updates everything, this will automate pinning the test requirements to the same version. Another possible solution that would sidestep needing this tool in the first place would be to just have pre-commit ci perform an upgrade on all the test requirements as well, but I don't think there is a way to specify a pre-commit hook to run that only runs on pre-commit.ci but not locally, unless one were to write a script to somehow identify something in the pre-commit.ci environment that is different from a local run. |
|
Thinking of merging this in a day or two if no other changes to make |
|
Oops I missed that this decreased coverage! I'll make a PR to fix that (just by adding a few coverage ignore comments...) |
In this pull request, we add a local pre-commit hook tool to keep the tool versions in
test-requirements.txtin sync with the equivalent versions in.pre-commit-config.yamlautomatically, meaning pre-commit.ci automatic updates are more seamless and there can never be cases where the test versions don't match the pre-commit versions.