Repository navigation
Add support for pcre / scripts / system hooks definition in .pre-commit-config.yaml #219
Description
Everything is in the title :)
Rationale: a pre-commit user shouldn't have to setup a git repository to configure a pre-commit check that can be defined in 5 lines or less.
Example: taken the from do_not_commit hook:
- id: do_not_commit
name: Block if "DO NOT COMMIT" is found
entry: DO NOT COMMIT
language: pcre
files: ^(.*)$
Suggested solutions:
- Allow for pcre / scripts / system hooks definition in .pre-commit-config.yaml
- Allow for the
repofield in .pre-commit-config.yaml to point to a subfolder (of the git repo configured withpre-commit) that contains a hooks.yaml .
This currently crashes becausepre-commitexpect to find git repository in the folder point byrepo.
Activity
asottile commentedon May 1, 2015
I think this is along similar lines of this: #173
And is kind-of possible: https://github.com/Yelp/venv-update/blob/a5960acab7101a1e70c57945b2038fef9d005aed/.pre-commit-config.yaml#L15-L22
I'd certainly like to make this easier though :)
Lucas-C commentedon May 1, 2015
Yes ! The
Yelp/venv-updateexample is exactly what I wanted !If you support this, maybe mention it in the docs ?
Although maybe allowing a
repo: localvalue would be cleaner than having to point it to an existing dummy github repo.asottile commentedon May 1, 2015
Yeah that way works currently, I wouldn't call it supported yet since it's kinda clunky and I want a more straightforward way of doing that. That method is unlikely to break though. I imagine the actual implementation will be similar
Lucas-C commentedon May 1, 2015
Ok. Then you can resolve this ticket if you want to track this feature elsewhere.
And thank you for your detailed answers !
Lucas-C commentedon May 5, 2015
I went on and hacked around it: what do you think of the solution described in #223 ?
Preparatory work & clean-up for pre-commit#219
Preparatory work & clean-up for pre-commit#219
Preparatory work & clean-up for pre-commit#219
Preparatory work & clean-up for pre-commit#219
Preparatory work & clean-up for pre-commit#219
Merge pull request #223 from Lucas-C/master
asottile commentedon May 8, 2015
I'm going to clarify here since I think I'm a bit confused :)
I'd like to make @bukzor's config work when it changes from:
to
I think (but I might be a bit off!) that all we would need to change to support this is:
Does this sound right? (mostly wanted to just get a checklist together so it's clear what the plan is here :D)
Lucas-C commentedon May 8, 2015
Sounds alright, + adding some tests :)
I won't have time to craft the commit today, but I plan to submit the pull request over the week-end.
bukzor commentedon May 8, 2015
Repo: null makes sense to me.
Just don't validate the sha for language: system.
Also: can we put system-style hooks at the top level? It doesn't actually
make sense to nest then under a repo at all.
Just ideas. If it makes things a mess, disregard.
--phone is hard.
On May 7, 2015 6:17 PM, "Anthony Sottile" notifications@github.com wrote:
Lucas-C commentedon May 8, 2015
Sounds nice & simple, but null / None is often a design smell: it is often better to use an "Optional" construct, a NullObject or the SpecialCase pattern. I'll stick to the magic string constant.
I don't understand what you meant here. I complete agree it doesn't make sense to put them in a repo, but why put them at the YAML config file hierarchy root ? I don't quite see what change to the plan @asottile detailed you are suggesting there. Could you further explain ?
asottile commentedon May 8, 2015
@bukzor this would entail a breaking change to the config file, I don't really want to do that at this time.
EDIT: null also seems fine to me as well
bukzor commentedon May 9, 2015
Is fine. It probably doesn't make sense for the list to be heterogeneous.
--phone is hard.
On May 8, 2015 9:54 AM, "Anthony Sottile" notifications@github.com wrote:
Merge pull request pre-commit#219 from pre-commit/upgrade_add_trailin…