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

Introduce .pre-commit-hooks.yaml as a replacement for hooks.yaml #470

Merged
merged 1 commit into from
Jan 22, 2017

Conversation

asottile
Copy link
Member

@asottile asottile commented Jan 21, 2017

I think this will make upstreams more likely to accept a PR which adds this metadata if we have a namespaced files.

(at the cost of an annoying migration -- sorry I chose a bad name in the project's infancy)

Is this a good idea?

Here's the output this generates when committing in this project:

$ git commit --amend
[WARNING] https://github.com/pre-commit/pre-commit-hooks.git uses legacy hooks.yaml to provide hooks.
In newer versions, this file is called .pre-commit-hooks.yaml
This will work in this version of pre-commit but will be removed at a later time.
If `pre-commit autoupdate` does not silence this warning consider making an issue / pull request.
[WARNING] https://github.com/pre-commit/pre-commit.git uses legacy hooks.yaml to provide hooks.
In newer versions, this file is called .pre-commit-hooks.yaml
This will work in this version of pre-commit but will be removed at a later time.
If `pre-commit autoupdate` does not silence this warning consider making an issue / pull request.
[WARNING] https://github.com/asottile/reorder_python_imports.git uses legacy hooks.yaml to provide hooks.
In newer versions, this file is called .pre-commit-hooks.yaml
This will work in this version of pre-commit but will be removed at a later time.
If `pre-commit autoupdate` does not silence this warning consider making an issue / pull request.
Trim Trailing Whitespace.................................................Passed
Fix End of Files.........................................................Passed
autopep8 wrapper.........................................................Passed
Check docstring is first.................................................Passed
Check JSON...........................................(no files to check)Skipped
Check Yaml...........................................(no files to check)Skipped
Debug Statements (Python)................................................Passed
Tests should end in _test.py.............................................Passed
Fix requirements.txt.................................(no files to check)Skipped
Flake8...................................................................Passed
Validate Pre-Commit Config...........................(no files to check)Skipped
Validate Pre-Commit Manifest.........................(no files to check)Skipped
Reorder python imports...................................................Passed

maybe a little too verbose? suggestions for something shorter?

@chriskuehl
Copy link
Member

I like the move and new name.

It does seem a little verbose but it's not too bad. It's an easy fix for hook authors.

My main concern would be: what happens when someone runs pre-commit autoupdate with an old version of pre-commit? The error message doesn't look super helpful:

(tmp.EhSQiYXzhI) ckuehl@raziel:/tmp/tmp.SU7lnAPqGN/repo$ pre-commit autoupdate
Updating /tmp/tmp.SU7lnAPqGN/hookrepo...[INFO] Initializing environment for /tmp/tmp.SU7lnAPqGN/hookrepo.
An unexpected error has occurred: InvalidManifestError: File /home/ckuehl/.pre-commit/repo_hwu50do/hooks.yaml does not exist

I'm not sure how to avoid that. Hook authors could duplicate (maybe symlink works?) hooks.yaml and .pre-commit-hooks.yaml during some transition period, or maybe make a fake hooks.yaml where every hook just has minimum_pre_commit_version: <new version>. Neither of those are great options but of course we can't just patch old versions of pre-commit.

@asottile
Copy link
Member Author

Yeah I thought about that too -- I think during a transition period hook authors would exactly duplicate the two files (as is done in this PR). I considered a symlink but it has worse portability (notably for windows). After the transition period, the minimum_pre_commit_version idea sounds good enough.

Copy link
Member

@chriskuehl chriskuehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems reasonable to me. transitions suck but I think this is a definitely positive change.

@asottile asottile merged commit 1a4c7aa into master Jan 22, 2017
@asottile asottile deleted the pre_commit_hooks_yaml branch January 22, 2017 01:09
asottile added a commit to pre-commit/pre-commit-mirror-maker that referenced this pull request Jan 22, 2017
asottile added a commit to pre-commit/pre-commit-hooks that referenced this pull request Jan 22, 2017
asottile added a commit to pre-commit/mirrors-yapf that referenced this pull request Jan 22, 2017
asottile added a commit to pre-commit/pre-commit-docker-flake8 that referenced this pull request Jan 22, 2017
asottile added a commit to pre-commit/mirrors-pylint that referenced this pull request Jan 22, 2017
asottile added a commit to pre-commit/mirrors-autopep8 that referenced this pull request Jan 22, 2017
asottile added a commit to pre-commit/mirrors-fixmyjs that referenced this pull request Jan 22, 2017
asottile added a commit to asottile-archive/cheetah_lint that referenced this pull request Jan 22, 2017
asottile added a commit to asottile/reorder-python-imports that referenced this pull request Jan 22, 2017
asottile added a commit to pre-commit/pre-commit.com that referenced this pull request Jan 22, 2017
@asottile
Copy link
Member Author

asottile commented Jan 22, 2017

I've gone ahead and updated all of our repositories (and all of my repositories) for this change.

I plan to create issues on projects on http://pre-commit.com/hooks.html something along the lines of:

Subject: pre-commit has changed from hooks.yaml -> .pre-commit-hooks.yaml

Hello pre-commit hook implementer!

In version 0.12.0 pre-commit has changed the default location for the file formerly known as hooks.yaml to make it more convincing for others to add more hooks.

As such, a migration has to (unfortunately) occur.

For maximum compatibility it is suggested to cp hooks.yaml .pre-commit-hooks.yaml (at least for the migration period). A copy is suggested over a symlink unless you do not care for windows compatibility (and I wouldn't blame you!).

Once the migration period is over (or you no longer care to support old versions of pre-commit), the hooks.yaml file is no longer necessary and may be deleted.

See #470 for more details

Thanks again for contributing to the pre-commit ecosystem, we couldn't do it without you :)
Anthony

This was referenced Jan 25, 2017
@asottile
Copy link
Member Author

/o\ I'm so sorry for all the email spam, please don't hate me :)

@Lucas-C
Copy link
Contributor

Lucas-C commented Jan 25, 2017

Question: I currently have both .pre-commit-config.yaml AND a hooks.yaml files in my repos.
E.g. https://github.com/Lucas-C/pre-commit-hooks-bandit
Is it ok to merge them ?

@chriskuehl
Copy link
Member

@Lucas-C the new file is .pre-commit-hooks.yaml, different from the config file

@Lucas-C
Copy link
Contributor

Lucas-C commented Jan 26, 2017

Oh sorry, silly me :)
Thanks

xqliang pushed a commit to xqliang/check-swagger that referenced this pull request Mar 2, 2017
jstewmon pushed a commit to jstewmon/check-swagger that referenced this pull request Mar 7, 2017
pre-commit/pre-commit#470 moved the default location of the hook
config file

bump version to 0.1.4

closes #1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants