Conversation
asottile
left a comment
There was a problem hiding this comment.
will have to fix CI but this is a good start
pre_commit/main.py
Outdated
| parser.add_argument( | ||
| '--new-head-ref', | ||
| help='The ref of the new HEAD (which may or may not have changed).', | ||
| ) |
There was a problem hiding this comment.
I think we could potentially reuse --source / --origin for these arguments?
There was a problem hiding this comment.
I can do that if you want to keep the arg count down. However, I don't think that source/origin maps directly to new-head-ref/prev-head-ref so it might create some confusion. I'll leave that decision up to you.
There was a problem hiding this comment.
source / origin is a "from ref" -> "to ref" relation, so I think it fits well enough
tests/conftest.py
Outdated
| with cwd(path): | ||
| cmd_output('git', 'add', '.') | ||
| git_commit() | ||
| yield path |
There was a problem hiding this comment.
since these are only used once, it probably makes sense to inline them adjacent to the test itself
There was a problem hiding this comment.
Do you want me to inline the entire fixture into the test or just the git add / git commit part?
There was a problem hiding this comment.
ah mostly that there isn't a reason to put it in conftest.py unless it is being shared across multiple tests
There was a problem hiding this comment.
Sounds good to me, I will move it.
|
Thanks for the feedback, I will fix the CI issues later today. |
|
@asottile I made the changes you requested and CI is now green as well, please take another look when you can. |
8247369 to
18fa004
Compare
|
Yeah, your welcome! It’s was fun, you’ve got a great project. Thanks for
all your input!
…On Sun, Feb 23, 2020 at 10:24 AM Anthony Sottile ***@***.***> wrote:
***@***.**** approved this pull request.
thanks again for working on this!
<https://camo.githubusercontent.com/da817a3f4b192dfb496051a9aba8111fb546ef29/68747470733a2f2f692e666c756666792e63632f673654523052564a4d4a3466356e577244523339744e7264646333674e53646d2e676966>
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1339?email_source=notifications&email_token=AAAQKANQQH36CAF2RZ2YQF3REK5L7A5CNFSM4KYWLCFKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCWSKWZA#pullrequestreview-363113316>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAQKALKSGBO55A3EQFZUXDREK5L7ANCNFSM4KYWLCFA>
.
|
|
I'm also going to follow up and add aliases for |
|
just played with this a bit, not sure if it's intentional but the I think they're not supposed to get any files at all so I'm making a change to fix that 👍 |
$ ~/workspace/pre-commit/venv/bin/pre-commit install -t post-checkout
pre-commit installed at .git/hooks/post-checkout
$ git add .
$ git commit -m 'add config'
[master (root-commit) d15280a] add config
1 file changed, 5 insertions(+)
create mode 100644 .pre-commit-config.yaml
$ git status
On branch master
nothing to commit, working tree clean
$ git status
On branch master
nothing to commit, working tree clean
$ git checkout master -b wat
Switched to a new branch 'wat'
identity.............................................(no files to check)Skipped
- hook id: identity
$ touch foo.py
$ git status
On branch wat
Untracked files:
foo.py
nothing added to commit but untracked files present
$ git add .
$ git commit -m "add foo"
[wat 1ca3ef0] add foo
1 file changed, 0 insertions(+), 0 deletions(-)
create mode 100644 foo.py
$ git checkout master
Switched to branch 'master'
identity.............................................(no files to check)Skipped
- hook id: identity
$ git checkout wat
Switched to branch 'wat'
identity.................................................................Passed
- hook id: identity
- duration: 0.04s
foo.py
$ cat .pre-commit-config.yaml
repos:
- repo: meta
hooks:
- id: identity
stages: [post-checkout] |
|
#1344 for that one! |
|
oh right -- this got released as part of 2.2.0 -- thanks again for the patch! |

This adds support for the
post-checkouthook.Fixes: #1120
Documentation PR: pre-commit/pre-commit.com#307
One thing I would still like to do, if possible, would be to force hooks like this to always have
always_runbe set since that is somewhat implied by the nature of the hook (since it doesn't operate on files).