Add `jsonschema.protocols.IValidator` by sirosen · Pull Request #890 · python-jsonschema/jsonschema

sirosen · 2021-12-09T23:57:20Z

I'm marking this as a draft. I wanted to share what I've got so far for #548, but I'm not certain that it's good to merge without checking in on typeshed to see what we should do there.

This is a Protocol implementation for type checking under mypy and other static analyzers. It uses the protocol class defined in py3.8+ and uses typing_extensions as a backport for py3.7

The documentation-only validator class has been replaced with the protocol, and docs are now driven via autoclass on the protocol.

Importantly, several documented methods of the class have been removed, as they were marked deprecated under jsonschema v3.0 and are no longer provided by the builtin validators.

Minor adjustments to the docs are made to repoint references at the new class definition.

docs/creating.rst

docs/validate.rst

codecov · 2021-12-10T17:19:49Z

Codecov Report

Merging #890 (3a62925) into main (0cb3a4c) will decrease coverage by 0.07%.
The diff coverage is 90.62%.

@@            Coverage Diff             @@
##             main     #890      +/-   ##
==========================================
- Coverage   98.33%   98.25%   -0.08%     
==========================================
  Files          19       20       +1     
  Lines        3125     3156      +31     
  Branches      378      425      +47     
==========================================
+ Hits         3073     3101      +28     
- Misses         41       44       +3     
  Partials       11       11

Impacted Files	Coverage Δ
jsonschema/_types.py	`100.00% <ø> (ø)`
jsonschema/validators.py	`97.24% <ø> (ø)`
jsonschema/protocols.py	`87.50% <87.50%> (ø)`
jsonschema/__init__.py	`86.66% <100.00%> (+0.95%)`	⬆️
jsonschema/tests/test_validators.py	`98.93% <100.00%> (+<0.01%)`	⬆️

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 0cb3a4c...3a62925. Read the comment docs.

Julian · 2021-12-10T17:20:56Z

Thanks! This looks very much in the right direction. The only "significantish" thing that occurs to me is perhaps it'd be nice to also now add verification that validators implement this protocol to CI -- I'm out of the loop on whether we need a pytype (or mypy) toxenv to do so but if you know, let me know. Basically any code appearing in the tests would be good to add before merging, to ensure the protocol correctly describes things.

sirosen · 2021-12-10T21:26:29Z

To keep us on the same page, here's the typeshed issue I just opened to discuss getting this added there:
python/typeshed#6564

it'd be nice to also now add verification that validators implement this protocol to CI

I very much agree. Because I marked it runtime_checkable, I should be able to write a test which asserts isinstance(validator_class(), jsonschema.protocols.Validator).
That won't check everything that a type checker cares about (limitations of runtime_checkable), but it's a good first step.

Doing more sophisticated mypy (or pytype or pyright) testing is possible, but it gets very messy (I'm happy to share details, maybe in a separate issue). Based on what I've seen doing it, and where I see jsonschema as being right now, I'd recommend against trying to do too much right now.

I'll look at setting up mypy on jsonschema itself, in a separate branch -- I think that will be necessary before we can think more about typing-related functionality. I don't want to force typing on anyone, but I'm happy to help try to make it useful here.

Julian · 2021-12-11T14:01:47Z

I very much agree. Because I marked it runtime_checkable, I should be able to write a test which asserts isinstance(validator_class(), jsonschema.protocols.Validator). That won't check everything that a type checker cares about (limitations of runtime_checkable), but it's a good first step.

Yes that was all I was hoping for but I see you sent the other PR for mypy which is also nice of you, will have a look. I'm not a huge fan overall you can imagine otherwise I'd have done so myself previously but I'm perfectly happy (and appreciative) with what you've done there so will likely merge both of these.

Julian

Thanks! Left some minor comments which you probably were addressing already.

There's also a .pyc file that snuck in here which you can please remove. After that should be good to merge, appreciated!

docs/validate.rst

jsonschema/protocols.py

docs/errors.rst

docs/faq.rst

docs/validate.rst

docs/creating.rst

jsonschema/protocols.py

This is a Protocol implementation for type checking under mypy and other static analyzers. It uses the protocol class defined in py3.8+ and uses typing_extensions as a backport for py3.7 The documentation-only validator class has been replaced with the protocol, and docs are now driven via autoclass on the protocol. Importantly, several documented methods of the class have been removed, as they were marked deprecated under jsonschema v3.0 and are no longer provided by the builtin validators. Minor adjustments to the docs are made to repoint references at the new class definition.

Co-authored-by: Julian Berman <Julian@GrayVines.com>

Primarily, rewrite `IValidator` to `Validator` Co-authored-by: Julian Berman <Julian@GrayVines.com>

sirosen · 2021-12-13T19:14:02Z

Thanks for the suggestions; it made updating that much easier!
I've done a rebase to get that pyc file out of the history (whoopsie), and I've marked this no longer a draft since I got a reply on typeshed that we should add this here first, and we'll get it into typeshed afterwards.

Let me add a few tests for isinstance checks, and hopefully this will be good to merge.
I'll revisit the mypy checking branch after this is in and do a rebase to make sure everything looks okay with both changes.

Several remaining cases referred to `IValidator` which is now just `Validtator`. Fix these. Add a test which ensures that all valdiators pass `isinstance(x, Validator)` using the runtime-checkable protocol.

This fixes `tox -e docs-style` `tox -e docs-linkcheck` is still failing (needs investigation)

sirosen · 2021-12-13T23:28:32Z

I've added the tests I wanted and gotten most of the build passing. I'm stuck on tox -e docs-linkcheck, which fails to find a link to jsonschema.protocols.Validator.iter_errors. I see an element in the dirhtml output which has that as its id, so I don't understand why it's failing.

I've fiddled with things a bit but couldn't get it to pass -- any help or feedback on how to make progress with that would be greatly appreciated.

Julian · 2021-12-13T23:34:49Z

Thanks! I'll have a look in the morning, Linkcheck failing (but not others) would be odd, linkcheck should only fail (by itself) if it's an external link, but will have to have a look. Appreciated!

Julian · 2021-12-15T12:38:22Z

Ah of course, linkcheck is failing because of the README change, which relies on RTD rebuilding the docs. That's fine we can merge and it will fail the first time but pass afterwards.

Thanks again for sending the PR!

Julian reviewed Dec 10, 2021

View reviewed changes

docs/creating.rst Outdated Show resolved Hide resolved

Julian reviewed Dec 10, 2021

View reviewed changes

docs/validate.rst Outdated Show resolved Hide resolved

Julian requested changes Dec 11, 2021

View reviewed changes

sirosen and others added 3 commits December 13, 2021 19:09

Update docs/validate.rst

5a99f6d

Co-authored-by: Julian Berman <Julian@GrayVines.com>

Apply suggestions from code review

642a09f

Primarily, rewrite `IValidator` to `Validator` Co-authored-by: Julian Berman <Julian@GrayVines.com>

sirosen force-pushed the add-validator-protocol branch from 50831dd to 642a09f Compare December 13, 2021 19:10

sirosen marked this pull request as ready for review December 13, 2021 19:11

sirosen added 2 commits December 13, 2021 21:22

Fix references to IValidator; add protocol tests

94dea8b

Several remaining cases referred to `IValidator` which is now just `Validtator`. Fix these. Add a test which ensures that all valdiators pass `isinstance(x, Validator)` using the runtime-checkable protocol.

Fix doc linting build

3a62925

This fixes `tox -e docs-style` `tox -e docs-linkcheck` is still failing (needs investigation)

Julian merged commit 4828032 into python-jsonschema:main Dec 15, 2021

Uh oh!

Conversation

sirosen commented Dec 9, 2021

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Dec 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Julian commented Dec 10, 2021

Uh oh!

sirosen commented Dec 10, 2021

Uh oh!

Julian commented Dec 11, 2021

Uh oh!

Julian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sirosen commented Dec 13, 2021

Uh oh!

sirosen commented Dec 13, 2021

Uh oh!

Julian commented Dec 13, 2021

Uh oh!

Julian commented Dec 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Dec 10, 2021 •

edited

Loading