-
Notifications
You must be signed in to change notification settings - Fork 10
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
Fix for check-manifest pre-commit hook in Python 3.12 #344
Conversation
Quality Gate passedIssues Measures |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #344 +/- ##
=======================================
Coverage 99.77% 99.77%
=======================================
Files 14 14
Lines 907 907
=======================================
Hits 905 905
Misses 2 2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this @sfmig!
Good to go as is, regarding your questions I don't have informed opinions, but here are my 2 cents anyway:
Right now, when we say we support Python 3.12 in practice we mean we run the code tests in Python 3.12.
I think that's what most Python projects mean, right?
But should we ensure other bits, like the developer tools, also run on the Python versions we support?
I'd say, yes. Our current docs for a development environment recommend Python 3.11
, just because we tend to go for the "middle" one of the 3 supported versions at any given point, out of convention. That said, I'd say it's a reasonable expectation that dev would work for all of our supported Python versions (hence why this PR is good).
If so, maybe it makes sense if we ensure that the CI actions related to precommits run the latest python version?
Maybe, to catch cases like this one? But where would you make this change, here or in the actions
repo? The problem with changing it in actions
is that projects that rely on those may differ in their supported Python versions?
@sfmig is this ready to be merged? |
yes! Apologies, I think I will open a PR in the actions repo / ask other team members tomorrow about this (but that belongs to the other repo) |
* update project overview * update mission statement * updated scope * update roadmaps and consistently use `movement` (monospace) * Add wheel as a dependency (#344) * implement Adam's suggestions * Apply some suggestions outright Co-authored-by: sfmig <33267254+sfmig@users.noreply.github.com> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * update project overview based on feedback * clarify statement about action recognition * updated scope * mention "keypoints" for SLEAP and DLC representations in "scope". --------- Co-authored-by: sfmig <33267254+sfmig@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Description
What is this PR
Why is this PR needed?
With Python 3.12,
pre-commit run check-manifest
returns an error of missing dependencies -- see more details in this issue.To fix it,
wheel
needs to be specified as a dependency ofcheck-manifest
in the pre-commit config.This error doesn't show up in CI because:
check-manifest
with Python 3.10 and without the--no-build-isolation
flag.Question
What does this PR do?
Adds
wheel
as a dependency tocheck-manifest
in the pre-commit config.References
The most relevant is this issue. from the cookiecutter repo.
How has this PR been tested?
Tests pass locally and in CI.
Is this a breaking change?
No.
Does this PR require an update to the documentation?
No.
Checklist: