-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Remove language_version
for pre-commit
#2430
Remove language_version
for pre-commit
#2430
Conversation
At my company, we set the Python version in `default_language_version` in each repo's `.pre-commit-config.yaml`, so that all hooks are running with the same Python version. However, this currently doesn't work for black, as the `language_version` specified here in the upstream `.pre-commit-hooks.yaml` takes precedence. Currently, this requires us to manually set `language_version` specifically for black, duplicating the value from `default_language_version`. The failure mode otherwise is subtle - black works most of the time, but try to add a walrus operator and it suddenly breaks! Given that black's `setup.py` already has `python_requires>=3.6.2`, specifying that `python3` must be used here isn't needed as folks inadvertently using Python 2 will get hook-install-time failures anyways. Remove the `language_version` from these upstream hook configs so that users of black are able to use `default_language_version` and have it apply to all their hooks, black included. Example `.pre-commit-config.yaml` before: ``` default_language_version: python: python3.8 repos: - repo: https://github.com/psf/black rev: 21.7b0 hooks: - id: black language_version: python3.8 ``` After: ``` default_language_version: python: python3.8 repos: - repo: https://github.com/psf/black rev: 21.7b0 hooks: - id: black ```
This isn't exactly correct. The current configuration successfully fixes a use case where a user is running pre-commit in Python 2 while also having Python 3 interpreter installed. Just noting. Not taking a stance on whether such legacy use case should still be supported 😄 |
For context, per pre-commit's maintainer, this works as intended. Clearly |
@hukkin that is technically true, but black's hooks already require a Ilya, I agree this the current setup appears to be working as documented by upstream, but I think this change will make black more ergonomic to use without sacrificing anything. |
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.
Please add an changelog entry but other than that this seems fine to me. Thank you!
P.S. for added convience here's a diff you can use:
diff --git a/CHANGES.md b/CHANGES.md
index a678aae..a4b8e01 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -7,6 +7,11 @@
- Add support for formatting Jupyter Notebook files (#2357)
- Move from `appdirs` dependency to `platformdirs` (#2375)
+### Integrations
+
+- The provided pre-commit hooks no longer specify `language_version` to avoid overriding
+ `default_language_version` (#2430)
+
## 21.7b0
### _Black_
I was going to push this to your branch, but I get a 403. Sounds like the box allowing maintainers to push to fork branches of PRs isn't ticked?
@ichard26 thanks for the review, I pushed a commit with that diff applied. |
Actually checking https://docs.github.com/en/github/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork it seems that option is only available for PRs from user-owned forks, which I made this PR from our org fork of the black repo. (I don't see the option to enable it on this PR.) TIL! |
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.
Ah yeah, didn't realize this was from an organization repository. No worries then, it's not worth the trouble even if it was technically feasible. I also didn't know that only users could permit that, you really do learn something everyday don't you heh.
Congrats on your first contribution to psf/black 🎉
… pre-commit to fix override of default_language_version psf/black#2430
It's not longer used. Bad copy/paste. Ref: psf/black#2430
It's not longer used. Bad copy/paste. Ref: psf/black#2430
This enables `default_language_version` stanzas in consuming `.pre-commit-config.yaml` files to take precedence. This change is the same as psf/black#2430, see that PR for full context on why this is beneficial. This was previously PR'ed to this project as #144. It was closed at the time so my company has been keeping a fork of this project with the patch applied. With the new maintainership, I wanted to ask whether this change would be accepted at this time. Thanks in advance for taking a look, happy to answer any questions.
This enables `default_language_version` stanzas in consuming `.pre-commit-config.yaml` files to take precedence. This change is the same as psf/black#2430, see that PR for full context on why this is beneficial. This was previously PR'ed to this project as adamchainz#144. It was closed at the time so my company has been keeping a fork of this project with the patch applied. With the new maintainership, I wanted to ask whether this change would be accepted at this time. Thanks in advance for taking a look, happy to answer any questions.
At my company, we set the Python version in
default_language_version
in each repo's
.pre-commit-config.yaml
,so that all hooks are running with the same Python version.
However, this currently doesn't work for black,
as the
language_version
specified herein the upstream
.pre-commit-hooks.yaml
takes precedence.Currently, this requires us to manually set
language_version
specifically for black,
duplicating the value from
default_language_version
.The failure mode otherwise is subtle -
black works most of the time,
but try to add a walrus operator and it suddenly breaks!
Given that black's
setup.py
already haspython_requires>=3.6.2
,specifying that
python3
must be used here isn't neededas folks inadvertently using Python 2 will get hook-install-time failures anyways.
Remove the
language_version
from these upstream hook configsso that users of black are able to use
default_language_version
and have it apply to all their hooks, black included.
Example
.pre-commit-config.yaml
before:After: