Skip to content

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

Merged
asottile merged 1 commit into
masterfrom
pre_commit_hooks_yaml
Jan 22, 2017
Merged

Introduce .pre-commit-hooks.yaml as a replacement for hooks.yaml#470
asottile merged 1 commit into
masterfrom
pre_commit_hooks_yaml

Conversation

@asottile

@asottile asottile commented Jan 21, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

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?

@asottile
asottile requested a review from chriskuehl January 21, 2017 21:19
@asottile
asottile force-pushed the pre_commit_hooks_yaml branch from d1c902a to b9e5184 Compare January 21, 2017 21:48
@chriskuehl

chriskuehl commented Jan 21, 2017

Copy link
Copy Markdown
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

asottile commented Jan 21, 2017

Copy link
Copy Markdown
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.

chriskuehl
chriskuehl approved these changes Jan 22, 2017
View reviewed changes

@chriskuehl chriskuehl left a comment

Copy link
Copy Markdown
Member

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
Add creation of .pre-commit-hooks.yaml
7151973 
See pre-commit/pre-commit#470
@asottile asottile mentioned this pull request Jan 22, 2017
Merged
asottile added a commit to pre-commit/pre-commit-hooks that referenced this pull request Jan 22, 2017
@asottile
Add .pre-commit-hooks.yaml for forward compatibility
7752c42 
See pre-commit/pre-commit#470
@asottile asottile mentioned this pull request Jan 22, 2017
Merged
asottile added a commit to pre-commit/mirrors-yapf that referenced this pull request Jan 22, 2017
@asottile
Add .pre-commit-hooks.yaml
23d9953 
See pre-commit/pre-commit#470
asottile added a commit to pre-commit/pre-commit-docker-flake8 that referenced this pull request Jan 22, 2017
@asottile
Add .pre-commit-hooks.yaml
e53b5af 
See pre-commit/pre-commit#470
asottile added a commit to pre-commit/mirrors-pylint that referenced this pull request Jan 22, 2017
@asottile
Add .pre-commit-hooks.yaml
2df8767 
See pre-commit/pre-commit#470
asottile added a commit to pre-commit/mirrors-autopep8 that referenced this pull request Jan 22, 2017
@asottile
Add .pre-commit-hooks.yaml
7263227 
See pre-commit/pre-commit#470
asottile added a commit to pre-commit/mirrors-fixmyjs that referenced this pull request Jan 22, 2017
@asottile
Add .pre-commit-hooks.yaml
68daa47 
See pre-commit/pre-commit#470
asottile added a commit to asottile-archive/cheetah_lint that referenced this pull request Jan 22, 2017
@asottile
Add .pre-commit-hooks.yaml
9055b23 
See pre-commit/pre-commit#470
asottile added a commit to asottile/reorder-python-imports that referenced this pull request Jan 22, 2017
@asottile
Add .pre-commit-hooks.yaml
62e75eb 
See pre-commit/pre-commit#470
asottile added a commit to pre-commit/pre-commit.com that referenced this pull request Jan 22, 2017
@asottile
Update make_all_hooks.py for .pre-commit-hooks.yaml
35bc82b 
See pre-commit/pre-commit#470
@asottile asottile mentioned this pull request Jan 22, 2017
Merged
@asottile

asottile commented Jan 22, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

chriskuehl added a commit to chriskuehl/puppet-pre-commit-hooks that referenced this pull request Jan 25, 2017
@chriskuehl
Rename hooks.yaml -> .pre-commit-hooks.yaml
67c9ecc 
In pre-commit/pre-commit#470, hooks.yaml was
renamed to .pre-commit-hooks.yaml to try to make the file more likely
for upstreams to include.

New versions of pre-commit will support both, but issue a warning about
the hooks.yaml name.

Moving it and setting up a symlink for compatibility should work on all
versions.
This was referenced Jan 25, 2017
Closed
Closed
Closed
Closed
Closed
Closed
Closed
This was referenced Jan 25, 2017
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@asottile

asottile commented Jan 25, 2017

Copy link
Copy Markdown
Member Author

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

@Lucas-C

Lucas-C commented Jan 25, 2017

Copy link
Copy Markdown
Contributor

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

chriskuehl commented Jan 25, 2017

Copy link
Copy Markdown
Member

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

@Lucas-C

Lucas-C commented Jan 26, 2017

Copy link
Copy Markdown
Contributor

Oh sorry, silly me :)
Thanks

xqliang pushed a commit to xqliang/check-swagger that referenced this pull request Mar 2, 2017
xiongqingliang
hooks.yaml -> .pre-commit-hooks.yaml
9b54201 
See pre-commit/pre-commit#470
jstewmon pushed a commit to jstewmon/check-swagger that referenced this pull request Mar 7, 2017
Jonathan Stewmon
copy hooks.yaml to .pre-commit-hooks.yaml
1078684 
pre-commit/pre-commit#470 moved the default location of the hook
config file

bump version to 0.1.4

closes #1
@asottile asottile mentioned this pull request Jun 24, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chriskuehl chriskuehl chriskuehl approved these changes

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@asottile @chriskuehl @Lucas-C