You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
I spent an hour or so doing a deep dive as the result of the following cryptic output (JSON schema validation failures on the yaml.) I guess in previous projects I'd always committed the yaml file before actually trying to run hooks.
An error has occurred: InvalidConfigError: Invalid content: .pre-commit-config.yaml
None is not of type u'array'
Failed validating u'type' in schema:
{u'items': {u'properties': {u'hooks': {u'items': {u'properties': {u'args': {u'items': {u'type': u'string'},
u'type': u'array'},
u'exclude': {u'default': u'^$',
u'type': u'string'},
u'files': {u'type': u'string'},
u'id': {u'type': u'string'},
u'language_version': {u'type': u'string'}},
u'required': [u'id'],
u'type': u'object'},
u'minItems': 1,
u'type': u'array'},
u'repo': {u'type': u'string'},
u'sha': {u'type': u'string'}},
u'required': [u'repo', u'sha', u'hooks'],
u'type': u'object'},
u'minItems': 1,
u'type': u'array'}
On instance:
None
Check the log at ~/.pre-commit/pre-commit.log
Eventually figuring out that this was the result of it not being able to actually load the yaml file, because it was being stashed.
I've since kicked myself a ton, but if we don't want to change it to not stash this yaml file, we could use a friendlier warning/error message and maybe a documentation PR as well. I'm willing to take a look. Thoughts?
I've been meaning to implement this, just haven't found the time. I also think that jsonschema needs dropping, it was useful at first when the schema was simple but as you can see the error messages suck :(. Though the custom validation will be more complicated, it'll probably be easier to add more complicated features and provide better error messages.
Hmm... I haven't looked at the code that does the git stash (I have a feeling that it's probably more annoying to say "stash everything except this one file" than might be expected) but should be able to pre-detect if the file doesn't exist before trying to run the json validation...which might lead to less confusion.
And then of course, there's always the least technical but probably quickest thing to do of shoving a giant warning into the docs :p
The way I would implement this is to grab the contents of the config file before stashing, stash, restore the contents of the config. Then when exiting the context, do the opposite.
Another idea would be to make it an error to have a modified and unstaged config file with a --allow-unstaged-config option. I think that would actually be the easiest to program and have the least edge cases. What do you think about that?
You mean it would be an error unles you specified that option, right? :)
Yea, that was kinda what I was thinking as a simple solution. I started thinking about our previous idea (mostly while getting familiar with pytest and the file system fixtures) and started to realize there are a lot of weird edge cases as well. :) We can always look into a better automatic way of handling it, removing json schema, whatever other things come to your mind later, but just putting a sensible warning is a good low hanging fruit.
So what I'll do is proceed with the error/warning if this modified/unstaged config exists. Then at least we can give a human error to this noob trap I ran into. Funnily enough, I had to explain this issue when I was trying to show someone how to set up pre-commit at work earlier...
Think I have something for the above, but going to give it a bit of testing, some sleep, and some better docs before I PR it. :p
Right now I'm just having it run a separate git diff against the config file and just spitting out various warnings/errors depending on the arg, before it actually does the existing diff + stashing
Cool cool, I imagine this being similar to the shortcut we do for unmerged files:
# pre_commit/commands/run.py
# Check if we have unresolved merge conflict files and fail fast.
if _has_unmerged_paths(runner):
logger.error('Unmerged files. Resolve before committing.')
return 1
Probably something like:
if not args.allow_unstaged_config and _has_unstaged_config(runner):
logger.error(
'.pre-commit-config.yaml is unstaged which can cause some unexpected behaviour. '
'To ignore this, run `git add .pre-commit-config.yaml`. '
'To silence this execute pre-commit with --allow-unstaged-config'
)
return 1
Activity
gabrielpugliese-luizalabs commentedon Sep 1, 2014
❤️
caffodian commentedon May 4, 2015
Dropping in to share my experience with this...
I spent an hour or so doing a deep dive as the result of the following cryptic output (JSON schema validation failures on the yaml.) I guess in previous projects I'd always committed the yaml file before actually trying to run hooks.
Eventually figuring out that this was the result of it not being able to actually load the yaml file, because it was being stashed.
I've since kicked myself a ton, but if we don't want to change it to not stash this yaml file, we could use a friendlier warning/error message and maybe a documentation PR as well. I'm willing to take a look. Thoughts?
asottile commentedon May 4, 2015
I've been meaning to implement this, just haven't found the time. I also think that jsonschema needs dropping, it was useful at first when the schema was simple but as you can see the error messages suck :(. Though the custom validation will be more complicated, it'll probably be easier to add more complicated features and provide better error messages.
caffodian commentedon May 4, 2015
Hmm... I haven't looked at the code that does the git stash (I have a feeling that it's probably more annoying to say "stash everything except this one file" than might be expected) but should be able to pre-detect if the file doesn't exist before trying to run the json validation...which might lead to less confusion.
And then of course, there's always the least technical but probably quickest thing to do of shoving a giant warning into the docs :p
asottile commentedon May 4, 2015
The relevant code is in https://github.com/pre-commit/pre-commit/blob/master/pre_commit/staged_files_only.py
The way I would implement this is to grab the contents of the config file before stashing, stash, restore the contents of the config. Then when exiting the context, do the opposite.
asottile commentedon May 5, 2015
Another idea would be to make it an error to have a modified and unstaged config file with a
--allow-unstaged-configoption. I think that would actually be the easiest to program and have the least edge cases. What do you think about that?caffodian commentedon May 6, 2015
You mean it would be an error unles you specified that option, right? :)
Yea, that was kinda what I was thinking as a simple solution. I started thinking about our previous idea (mostly while getting familiar with pytest and the file system fixtures) and started to realize there are a lot of weird edge cases as well. :) We can always look into a better automatic way of handling it, removing json schema, whatever other things come to your mind later, but just putting a sensible warning is a good low hanging fruit.
So what I'll do is proceed with the error/warning if this modified/unstaged config exists. Then at least we can give a human error to this noob trap I ran into. Funnily enough, I had to explain this issue when I was trying to show someone how to set up pre-commit at work earlier...
caffodian commentedon May 6, 2015
Think I have something for the above, but going to give it a bit of testing, some sleep, and some better docs before I PR it. :p
Right now I'm just having it run a separate git diff against the config file and just spitting out various warnings/errors depending on the arg, before it actually does the existing diff + stashing
asottile commentedon May 6, 2015
Cool cool, I imagine this being similar to the shortcut we do for unmerged files:
Probably something like:
improvement for pre-commit#157 - error if an unstaged config exists a…
pre-commit#157 - test modifying and not staging the config in a passi…
fix for pre-commit#157
Fix for pre-commit#157 (with upstream changes)
asottile commentedon May 10, 2015
Closed via #225