Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check if --config option is a path to a regular file #1093

Closed
wants to merge 4 commits into from
Closed

Check if --config option is a path to a regular file #1093

wants to merge 4 commits into from

Conversation

geieredgar
Copy link
Contributor

This change implements the suggestion made by @asottile in #1037
I tried to be compatible to the current behavior of _adjust_args_and_chdir(args), therefore we also check if the path points to a file relative to the git root directory.

If there is some interest in getting this merged I will add/fix the required tests.

@asottile
Copy link
Member

hmmm this is interesting, I'll have to think about this more

the interesting / challenging part of this is there's commands that accept --config but don't do anything with it (gc / clean / sample-config, and maybe probably the brand new init-templatedir probably should as well) -- in these cases it isn't super important that .pre-commit-config.yaml resolves to a file which exists, but sometimes (rarely) that argument is used just as a string

the other problem with doing more clever parsing at the argparse stage is we don't properly have enough error handling set up to produce a useful trace (for instance, if a command is called when outside of a git repository)

I'll have to put more thought into this 🤔

@geieredgar
Copy link
Contributor Author

@asottile Now the path checking should be disabled for gc / clean / sample-config and init-templatedir.

It now also handles the case of being run outside of a git repository by catching CalledProcessError (This is rather hacky, it would be probably better to have a utility function that checks whether or not we are inside a git repo.)

@asottile
Copy link
Member

sorry I haven't gotten back to this, still thinking about whether eagerly or waiting for failure is the right approach here (especially as it invokes git more than I'd like to in the critical path -- and for things which previously didn't actually load the configuration / validate it)

I'm leaning towards closing this and wontfixing the original issue -- thank you either way for the PR though <3

@asottile
Copy link
Member

I think for now I'm going to close this and the associated issue :( -- the case is rare enough and the change potentially breaks some legitimate workflows. thank you again for taking the time to work on this and I'm sorry it didn't work out in the end <3

@asottile asottile closed this Dec 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants