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

pre-commit hook argument for schemafile doesn't support ~/ #9

Closed
dvankampen opened this issue Jul 20, 2021 · 7 comments
Closed

pre-commit hook argument for schemafile doesn't support ~/ #9

dvankampen opened this issue Jul 20, 2021 · 7 comments

Comments

@dvankampen
Copy link

If we have a schemafile in the user's home directory, I can pass ~/schemafile.json when invoking check-jsonschema from the command line without error, but if I pass args: ["--schemafile", "~/schemafile.json"] to the pre-commit hook, I get a FileNotFoundError, because it does not expand the ~/ operator. Looks like the call to open could use a os.path.expanduser on the path being opened? This allows multiple developers with different usernames to have the schemafile in their home directory, without having to have the username hard-coded in the pre-commit yaml file.

@Borda
Copy link
Contributor

Borda commented Aug 1, 2021

you can try to replace ~ by $HOME

@dvankampen
Copy link
Author

Just tried $HOME, tried ${HOME}, tried \$HOME (in case the $ needed escaping), they all produce a FileNotFoundError. It is because it is simply using an open call
image
which doesn't support environment variables or bash conventions like ~.

@Borda
Copy link
Contributor

Borda commented Aug 2, 2021

right, there shall be os.path.expanduser(args.schemafile)

@sirosen
Copy link
Member

sirosen commented Aug 2, 2021

I think there's a fair case to be made that the current behavior is correct. The shell -- or whatever tool is calling check-jsonschema -- is responsible for any env var or tilde expansion prior to passing args.
I'm not strongly opposed to adding an expanduser call, as it does no harm in 99.9% of cases, but before I add anything I want to check if a simple change in your usage will suffice.

pre-commit does not do any environment munging -- in fact, the maintainer has been pretty clear about that -- so $HOME should be preserved. Rather than having check-jsonschema do the expansion for you, why not ask an expert, bash?
Simply replace the entry with a bash invocation which expands $HOME for you. e.g.

- repo: https://github.com/sirosen/check-jsonschema
  rev: 0.4.0
  hooks:
    - id: check-jsonschema
      files: foo.json
      entry: bash -c 'check-jsonschema --schemafile "$HOME/.cache/jsonschema_validate/github-workflow" "$@"' --

I will warn you to be cautious in general relying on uncontrolled data in people's homedirs. It strikes me as likely to cause issues -- but your workflow, your rules. 😄

As an alternative workaround, there's always the "escape valve" of adding a local script hook and putting whatever logic you want into a bash script. Just pass $@ through to check-jsonschema and you can still have pre-commit select which filenames to pass.

If neither of these work, we can consider expanduser, but I'd rather avoid making that change.

@dvankampen
Copy link
Author

@sirosen thank you so much for the well thought out reply! Your suggestions seem promising. The issue I have now with "$@" is that it doesn't handle filenames with whitespace, but I can track that down, I think.

@sirosen
Copy link
Member

sirosen commented Sep 30, 2021

I haven't heard more about this, but I'm confident that this can be solved without a change in check-jsonschema, at least for pre-commit usage.

That said, I'm looking back at this and think I'm being too rigid. (I often find it frustrating when maintainers choose "purity" rather being pragmatic and helping me. Now I'm being that guy. 😂 )

os.path.expanduser is documented as not doing anything weird, and I'd rather do that than expandvars ($HOME support) because it could definitely produce some surprising results.
I'll make this change and get started on a testsuite for check-jsonschema, perhaps as early as this weekend.

@sirosen
Copy link
Member

sirosen commented Oct 3, 2021

I've released v0.5.0 . It adds a call to expanduser() which should allow you to call

$ check-jsonschema --schemafile '~/foo.json' ...

and see the ~ expand.

0.5.0 was a fairly extensive refactor, aimed at making more of the code testable. There are also some new flags (--no-cache and --default-filetype), which will hopefully be useful. Please open issues if you see any problems!

@sirosen sirosen closed this as completed Oct 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants