Skip to content
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

Merged
merged 2 commits into from Aug 18, 2021

Conversation

aneeshusa
Copy link
Contributor

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

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
```
@hukkin
Copy link
Contributor

hukkin commented Aug 16, 2021

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.

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 😄

@ikonst
Copy link

ikonst commented Aug 17, 2021

For context, per pre-commit's maintainer, this works as intended. Clearly language_version is a bit of a blunt instrument here, if the desired outcome is more like a version constraint, but that's what we have to work with.

@aneeshusa
Copy link
Contributor Author

@hukkin that is technically true, but black's hooks already require a minimum_pre_commit_version of 2.9.2, which is after pre-commit itself dropped Python 2 support. So, pre-commit will already be running under Python 3, and in the absense of a langauge_version defined here, will reuse the same Python it is running under for the black hook. So I don't think that use case actually works today with the latest black hook.

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.

Copy link
Collaborator

@ichard26 ichard26 left a 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?

@aneeshusa
Copy link
Contributor Author

@ichard26 thanks for the review, I pushed a commit with that diff applied.
I'll check internally about the box to allow maintainers to push to fork branches, not sure if we have a policy on that yet but I know they are careful about tracking provenance.

@aneeshusa
Copy link
Contributor Author

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!

Copy link
Collaborator

@ichard26 ichard26 left a 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 🎉

@ichard26 ichard26 merged commit ef7c45f into psf:main Aug 18, 2021
neutrinoceros added a commit to neutrinoceros/black that referenced this pull request Sep 6, 2021
AlexanderVR added a commit to AlexanderVR/dbt-duckdb that referenced this pull request Oct 31, 2022
… pre-commit to fix override of default_language_version psf/black#2430
gsilvapt added a commit to surface-security/surface that referenced this pull request Apr 10, 2023
It's not longer used. Bad copy/paste.

Ref: psf/black#2430
gsilvapt added a commit to surface-security/surface that referenced this pull request Apr 10, 2023
It's not longer used. Bad copy/paste.

Ref: psf/black#2430
adamchainz pushed a commit to adamchainz/blacken-docs that referenced this pull request Aug 16, 2023
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.
adamchainz pushed a commit to maxb2/blacken-docs that referenced this pull request Aug 16, 2023
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.
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.

None yet

6 participants