Preparatory work & clean-up for #219 - #223

I like the idea.

I wonder if we can simplify this further and reuse existing logic by making local-hooks slightly less special to configure:

• Make a special value for repo (for instance pre-commit-locally-configured or maybe something less verbose)
• If that special repo is encountered, its backing manifest is a list which always contains all hook ids with basically a Null configuration
• The code already allows overriding of all the keys presented in the backing configuration so the rest would just work

I also like the cleanup work that was done for _run_hooks it might be easier to think about this review if we split this into two PRs as well

👍

Agreed, I'll split it in two commits.

Your solution sounds good, but it would make the sha field no more mandatory. In the JSON schema. But we could add an extra validation step so it's not a blocker.

I'm ok to move to this solution, but don't you think the current JSON schemas definition kind of overlap ? There is some code duplication for the allowed overrides, I'll try to make it more DRY.

Yeah the json schemas do kind of overlap. I really hope to get rid of them eventually (as we've discussed in the other issue) and replace them with a bunch of functions.

imo aim for correctness and readability instead of working hard at DRY, I've found that in a lot of cases expressiveness / correctness kind of work against DRY-ness. Though I think replacing the jsonchemas will actually make a lot of things cleaner / more flexible but that's definitely out of scope for this change I would think :)

I'm cool with making sha optional as long as we require it in the normal case

A comment on something you wrote "The code already allows overriding of all the keys presented in the backing configuration so the rest would just work" : actually the hooks definition in the manifest & config schemas differ quite a bit : some fields actually cannot be overridden, only id is required in the config, and it also has an additional exclude field, etc. Due to this differences (manly the missing fields) I feel like we cannot actually implement "local hooks" without modifying the schema.
What would you thing of something like this in validate_config.py :

from pre_commit.clientlib.validate_manifest import MANIFEST_JSON_SCHEMA
CONFIG_JSON_SCHEMA = {
    'type': 'array',
    'minItems': 1,
    'items': {
        'type': 'object',
        'properties': {
            'repo': {'type': 'string'},
            'sha': {'type': 'string'},
            'hooks': MANIFEST_JSON_SCHEMA
        },
        'required': ['repo', 'hooks'],
    }
}

With possibly more tweaking (like adding the exclude field to the manifest).

I updated the pull request to only contains preliminary changes.
Please don't merge it until you answer my question above :)

Also you wrote "I really hope to get rid of [JSON schemas] eventually, as we've discussed in the other issue". I don't think you mentioned that before in an issue I was part of. Are you refering to issue #157 or something else ?

Coverage remained the same at 100.0% when pulling a5259ab on Lucas-C:master into aec1c55 on pre-commit:master.

I'll comment on the second thing when I'm not on the phone :D

The first however: though the fields are not specified in the config schema, the way the runtime hook dictionary is made is by merging the config dict on top of the manifest dict. JSON schema is a bit permissive in that it let's you add keys even if they aren't in the schema.

Address comment for pre_commit/runner.py and I'll merge

👍

Ah yeah as for jsonschema, I totally got you confused with another person. I'm terrible sorry about that >.< Here's the comment I mentioned it in: #157 (comment)

the way the runtime hook dictionary is made is by merging the config dict on top of the manifest dict.

Are you refering to the following code snippet ?
https://github.com/pre-commit/pre-commit/blob/master/pre_commit/repository.py#L48

Then ok, but we won't be able to reuse that code for the local hooks, as it's nested in the Repository class.
But we can simply copy that merge logic.

Coverage remained the same at 100.0% when pulling 7dd8c6a on Lucas-C:master into aec1c55 on pre-commit:master.

Oh forgot to mention, please PR to add your name on http://github.com/pre-commit/pre-commit.github.io :D

Thanks for contributing!