pre-commit integration v2

[AleksaC]

Here's an improved version of pre-commit integration. I think it's in accordance with what @janosh and @asottile proposed in #765 at least in terms of interface. I made a test repo with pre-commit github action where you can check how the hook works on a simple test example. Any feedback is welcome.

[codecov]

Codecov Report

Merging #857 (851b4fd) into main (0878727) will increase coverage by 0.03%.
The diff coverage is 98.77%.

❗ Current head 851b4fd differs from pull request most recent head a03e77f. Consider uploading reports for the commit a03e77f to get more accurate results

@@            Coverage Diff             @@
##             main     #857      +/-   ##
==========================================
+ Coverage   98.25%   98.29%   +0.03%     
==========================================
  Files          20       22       +2     
  Lines        3152     3519     +367     
  Branches      423      471      +48     
==========================================
+ Hits         3097     3459     +362     
- Misses         44       47       +3     
- Partials       11       13       +2     

Impacted Files	Coverage Δ
jsonschema/_pre_commit_cli.py	93.47% <93.47%> (ø)
jsonschema/tests/test_pre_commit_cli.py	100.00% <100.00%> (ø)
jsonschema/validators.py	97.60% <0.00%> (+0.36%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0878727...a03e77f. Read the comment docs.

[janosh]

Great work!

Is it necessary to release this hook with a new id (jsonschema-v2)? Perhaps its release can coincide with a major version bump of jsonschema so users know to expect breaking changes? Of course if none is planned in the foreseeable future, this is impractical.

[janosh]

type=str is the default.

[janosh]

Is this line necessary?

[AleksaC]

Yeah in case argv is None which is the default value, I guess I could use sys.argv[1:] as the default instead

[janosh]

Could you leave a comment why we need to check both arg != "--schema" and arg != parsed_args.schema?

[AleksaC]

The goal here is to filter out all arguments other than jsonschema cli options. So when we have --schema schema.json argv will contain both --schema and schema.json (parsed_args.schema)

[asottile]

the implementation seems very fragile -- I would write a fresh argument parser that does precisely what you want instead of trying to pick apart the other one.

the arg filtering for --schema is also very fragile as it doesn't handle prefix matching or --schema=...

[asottile]

the old one should be left for compatibility at least for some time

[Julian]

Let's call the new one "validate", in case we some day have some hook that does something different (and also since I hate names with versions in them).

[AleksaC]

Thanks for the feedback @asottile.

I considered writing a fresh parser and the reason I didn't is because I didn't want to cause additional maintenance burden or make big changes to the existing one.

Writing a fresh parser would mean that I'd need to duplicate a decent portion of the original parser. In addition to that run and parse_args functions expect arguments as they are parsed by the original parser so I would need to either transform the args to match the format which would be fragile in a similar way the current implementation is or to once again copy the functions and make small modifications to them. Having two parsers that are basically the same would mean that changes to the cli would need to be made in two places.

The other option would be to rewrite a portion of the original parser so that it could be reused for the new one but reusing arguments in argparse isn't clean and I wanted to avoid changing the existing code.

I didn't like the current implementation either, but I couldn't think of a way it breaks other than major change happening to the original parser like changing positional arguments or renaming instances option. There are two other assumptions about argparse itself, which from my experience with argparse and brief glance at its source code seem to be true. First assumption is that the first action is help. Second assumption is that actions are stateless meaning that the previous invocations won't mess with the later invocations.

When it comes to filtering other than the case I missed with schema there is one case where it can break - if an argument to some other option matches schema and or one of the files in which case it would be filtered out. The only such option currently is --instances so I probably needed to forbid its usage by throwing an exception if it is provided. I can't think of an option that may be added in the future that would also suffer from this problem.

[Julian]

I'll have to read more closely to form an opinion on shoehorning into the command line interface or not -- my intuition though is that given how simple this hook is likely to be, that it's easier not to shoehorn it, and just to directly implement the interface needed for nice pre-commit usage on top of the normal jsonschema APIs (that the other CLI uses).

This CLI (for pre-commit) wouldn't be directly public anyhow, it'd just be used via pre-commit.

But yeah that's just my first intuition, so I could be wrong after looking more carefully.

And thanks again for doing this!

[Julian]

Let's call the new one "validate", in case we some day have some hook that does something different (and also since I hate names with versions in them).

[Julian]

Can we name the module a valid identifier? _cli_pre_commit I guess is fine.

[Julian]

Once the interface is agreed upon I definitely wouldn't mind if this file ran in CI, even if not all the code paths were covered, just to ensure the hook doesn't stop working due to a change.

[AleksaC]

Here's a fresh parser implementation of the pre-commit cli. I brought back the old hook and renamed the new one not to contain version in it. I also adapted the tests for original cli for the pre-commit cli. I added pycache files to .gitignore since I commited them accidentally (can revert this if you prefer it in a separate pr).

[janosh]

Feels like a lot of work went into this already. Anything I can do help get this in?

[Julian]

So sorry, somehow I didn't realize this was waiting on me! Will have another look, though will likely take a few days to clear some other things off.

[ssbarnea]

Any chance to revive this PR?

[sirosen]

Per #910, we're going to start treating check-jsonschema as the official CLI for jsonschema. One of the responsibilities of that project is providing a set of pre-commit hooks. The generic check-jsonschema hook is comparable to the behavior of this PR.

I would recommend closing this. I know that people have worked hard on it -- and I don't feel great about recommending that the work be discarded -- but I believe that there's a better future for pre-commit hooks in a separate project from jsonschema itself. If there are valuable behaviors here which are missing from check-jsonschema, I'm happy to take PRs, feature requests, bug reports, or constructive criticism!

[Julian]

I would recommend closing this. I know that people have worked hard on it -- and I don't feel great about recommending that the work be discarded -- but I believe that there's a better future for pre-commit hooks in a separate project from jsonschema itself.

It's my apology to make, I'm sorry all for not getting to this :( some other (non-jsonschema things) have been sapping even more time away.

But yes agreed, we're going to move this functionality there, so please give feedback there.