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

pre-commit sample configuration should not suggest rev: stable #420

Closed
asottile opened this issue Jul 23, 2018 · 10 comments · Fixed by #2416
Closed

pre-commit sample configuration should not suggest rev: stable #420

asottile opened this issue Jul 23, 2018 · 10 comments · Fixed by #2416
Labels
C: integrations Editor plugins and other integrations

Comments

@asottile
Copy link
Contributor

👋 hello, pre-commit maintainer here :)

The sample configuration currently suggests rev: stable for black's repository setup. A mutable ref poses some problems. Notably it gives the illusion that the latest version is being used but in reality it uses the version at install time.

This setup isn't suitable for those looking for a repeatable and reproducible linting experience.

The suggestion that we use for our official repositories is to either list the version explicitly in the docs (though this adds tedium to the release cycle) or to use some suitable non-value with instructions on how to substitute an appropriate value.

Yet another option would be to suggest pre-commit autoupdate after writing such a configuration as this will usually convert the mutable rev to an immutable tag (or sha if --bleeding-edge is passed).

@ambv
Copy link
Collaborator

ambv commented Aug 17, 2018

Thanks for reaching out! Sorry for staying quiet, I was happily crousing through flyover states when you wrote this.

I'm intrigued by the autoupdate option, I didn't really think much about the moving tag not actually updating on people's pre-commit installations. This is a problem that we need to address.

@asottile
Copy link
Contributor Author

No problem! travel is good!

I can take a stab at this if you'd like, depends on what option you'd like to call out in the docs -- I can help with a PR if you'd like too!

@ambv
Copy link
Collaborator

ambv commented Aug 17, 2018

Would be great if you fixed that for us, sure :-) First, I'd like to understand what you propose we should do.

@asottile
Copy link
Contributor Author

I don't want to add more release burden for you so the "list the exact version and bump it as released" seems probably out of the question. Using a suitable non-value is straightforward, but leads to a huge amount of SEO for this issue for some reason.

Probably the best solution is to keep the current sample configuration but then right before the "pre-commit install" suggest something along the lines of:

To get a repeatable installation make sure to change the moving tag `stable` to a release of
`black`.  The easiest way to do this is by running `pre-commit autoupdate`
(or `pre-commit autoupdate --repo https://github.com/ambv/black`).

@zsol
Copy link
Collaborator

zsol commented Aug 17, 2018

@asottile have you considered solving this from pre-commit itself? That seems a better place to solve this, as I imagine any project providing a precommit config will run into this problem.

Maybe the commit hook could check if there's a new version and print a warning/offer to upgrade? Hey, your hooks passed but you have 3 plugins that are outdated. Here's how to update them: ... Here's how to disable this message: ...

@asottile
Copy link
Contributor Author

I've hesitated because it adds two things I don't want:

  • slowness
  • phone home

@ambv
Copy link
Collaborator

ambv commented Aug 17, 2018

What Zsolt is suggesting could be opt in (and I would advertise this in my README) and could happen asynchronously while hooks are running.

@asottile
Copy link
Contributor Author

yeah I'm not saying I'm opposed to adding something like it, but it's not complexity I've decided to take on yet (there's also the whole question of making async work gracefully in python2, exposing errors from it, preempting if hooks finish faster, not taking resources, adding an option to opt into it, documenting it, messaging for it, and a whole slew of other considerations). It's not a simple feature

@jmknoble
Copy link

asottile:

Yet another option would be to suggest pre-commit autoupdate after writing such a configuration as this will usually convert the mutable rev to an immutable tag (or sha if --bleeding-edge is passed).

I'm in favor of that. If someone's adding a hook for use with pre-commit, they should pretty much autoupdate anyway.

You could even, if you wish, recommend:

pre-commit autoupdate --repo https://github.com/ambv/black

as that would have localized effect (for black only).

@beenje
Copy link

beenje commented Aug 30, 2019

Just ran in this issue. I was using "stable" but my local version of black and the one on my ci was different making my test to fail.

The pre-commit documentation clearly states that mutable rev are not supported:

pre-commit assumes that the value of rev is an immutable ref (such as a tag or SHA) and will cache based on that. Using a branch name (or HEAD) for the value of rev is not supported and will only represent the state of that mutable ref at the time of hook installation (and will NOT update automatically).

Reading this issue 974, it doesn't look like it's gonna change.
Would be nice to have black documentation in sync.

matthewfeickert added a commit to scikit-hep/pyhf that referenced this issue Aug 31, 2020
* Apply Black v20.8b1 formatting
   - Should be similar to format of first stable release of Black
* Update Black pre-commit hook to use rev v20.8b1
   - Using pinned revs is the recommended approach for pre-commit: psf/black#420
* Update development docs to advocate for and explain the use of pre-commit autoupdate
matthewfeickert added a commit to pyhf/pyhf-validation that referenced this issue Sep 21, 2020
* Apply Black v20.8b1 formatting
   - Should be similar to format of first stable release of Black
* Update Black pre-commit hook to use rev v20.8b1
   - Using pinned revs is the recommended approach for pre-commit: psf/black#420
matthewfeickert added a commit to scikit-hep/pylhe that referenced this issue Sep 22, 2020
* Update Black pre-commit hook to use rev v20.8b1
   - Using pinned revs is the recommended approach for pre-commit: psf/black#420
* Update flake8 pre-commit hook to use rev v3.8.3
jnothman added a commit to Weed-AI/Weed-AI that referenced this issue Sep 24, 2020
Per psf/black#420 adopt a fixed tag. CI can tell us when the code style is out of date with latest black version.
hlydecker pushed a commit to Weed-AI/Weed-AI that referenced this issue Sep 25, 2020
Per psf/black#420 adopt a fixed tag. CI can tell us when the code style is out of date with latest black version.
bockstaller added a commit to bockstaller/Python-Default that referenced this issue Nov 12, 2020
I've run precommit-autoupdate to update black to a current version with an explicit version number. See this issue for reference psf/black#420 (comment)
This allows us to pin the github-actions tool versions as well.

This commit applies the new formatting style directly.
jGaboardi added a commit to jGaboardi/spaghetti that referenced this issue Jan 29, 2021
chinnichaitanya added a commit to chinnichaitanya/python-discord-logger that referenced this issue Feb 7, 2021
The recent revisions of the pre-commit integration with black has specified to use the version instead of a constant branch name like `stable` so that it could be updated seemlessly using `pre-commit autoupdate`. For reference,

- https://pre-commit.com/#using-the-latest-version-for-a-repository
- pre-commit/pre-commit#1354
- https://github.com/psf/black#version-control-integration
- psf/black#420
chinnichaitanya added a commit to chinnichaitanya/python-slack-logger that referenced this issue Feb 7, 2021
The recent revisions of the pre-commit integration with black has specified to use the version instead of a constant branch name like `stable` so that it could be updated seemlessly using `pre-commit autoupdate`. For reference,

- https://pre-commit.com/#using-the-latest-version-for-a-repository
- pre-commit/pre-commit#1354
- https://github.com/psf/black#version-control-integration
- psf/black#420
widdowquinn pushed a commit to widdowquinn/pyani that referenced this issue May 27, 2021
As inidicated in psf/black#420 the mutable tag is not appropriate,
so we change to pin a fixed version tag.

Additionally, we remove the black version dependency from
blacken-docs, and add blacken-docs to the requirements-dev.txt file.
lfit-replication pushed a commit to lfit/releng-docs-conf that referenced this issue May 29, 2021
pre-commit black current configuration uses stable for rev, what creates
warnings at pre-commit runtime as discussed at the following URL
psf/black#420

"pre-commit assumes that the value of rev is an immutable ref (such as a
tag or SHA) and will cache based on that. Using a branch name (or HEAD)
for the value of rev is not supported and will only represent the state
of that mutable ref at the time of hook installation (and will NOT
update automatically)" as stated in the official documentation at
https://pre-commit.com/#using-the-latest-version-for-a-repository

rev stable has been replaced here by 21.5b1 in the precommit yaml file
to fix the warning and ensure CI and local pre-commit coherency.

Signed-off-by: Guillaume Lambert <guillaume.lambert@orange.com>
Change-Id: I577d22e56155212bbb708213efb8f09c0283dd64
widdowquinn pushed a commit to widdowquinn/pyani that referenced this issue Jun 22, 2021
As inidicated in psf/black#420 the mutable tag is not appropriate,
so we change to pin a fixed version tag.

Additionally, we remove the black version dependency from
blacken-docs, and add blacken-docs to the requirements-dev.txt file.
felker added a commit to felker/deephyper_pytorch_layers that referenced this issue Jul 19, 2021
Dont use rev: stable
suggestion from https://github.com/psf/black/blob/main/docs/integrations/source_version_control.md

psf/black#420

Also black wont fix comments that are too long
psf/black#1713
psf/black#182

Must manually fix these to stop flake8 from complaining
carlos-villavicencio-adsk added a commit to shotgunsoftware/tk-ci-tools that referenced this issue Aug 17, 2023
carlos-villavicencio-adsk added a commit to shotgunsoftware/tk-ci-tools that referenced this issue Aug 21, 2023
* Fix XML coverage reporting

* Bump Python version to code style validation

* Don't use stable for black per psf/black#420

* Migrated pre-commit config
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: integrations Editor plugins and other integrations
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants