PR for Issue 728

[miquelougoogle]

This is an example on how to to enhance best_match function, to prioritize issues of same type. I'm not sure where the code should go, and how it should be separated between the different files. This simply is an easy way to fix the problem, and I expect you'll be able to suggest how best to include this in the overall architecture of your project.

Definitions of non draft-07 validation types have not been included here, since I expect there are things that can be improved before with this PR.

What do you think.

[codecov]

Codecov Report

Merging #738 into master will increase coverage by 0.03%.
The diff coverage is 98.03%.

@@            Coverage Diff             @@
##           master     #738      +/-   ##
==========================================
+ Coverage   96.02%   96.06%   +0.03%     
==========================================
  Files          17       17              
  Lines        2664     2716      +52     
  Branches      310      323      +13     
==========================================
+ Hits         2558     2609      +51     
  Misses         87       87              
- Partials       19       20       +1     

Impacted Files	Coverage Δ
jsonschema/exceptions.py	92.98% <97.14%> (+0.95%)	⬆️
jsonschema/tests/test_exceptions.py	100.00% <100.00%> (ø)
jsonschema/_validators.py	99.54% <0.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 ddcc2e2...79634da. Read the comment docs.

[Julian]

Thanks for giving this a shot it's super appreciated! I haven't gotten a chance to think about this carefully, but two quick thoughts --

First, it's probably good if we start by adding the test cases we'll care about improving. There are a few I know in that issue ticket.

Second -- I don't want to be hardcoding types (which type a validator cares about) especially since that'll be very brittle in the face of either TypeChecker changes or additional validators being defined -- instead I think we should be able to do things like inspect whether the schema has type inside of it, and then check to see whether branches of a compound error match or do not match that type. It'll be easier I think once we've got the tests in place, but hopefully that makes some bit of sense already?

And again really appreciate you taking a crack at this given it'd be unlikely I get to it otherwise.

[miquelougoogle]

Hey,
Thank you for your comment, and for your support, I have added some tests so you can maybe better see what is the purpose of the PR. These tests are only to check the behaviour of the best_match and does not cover the rest, since I think that it can be improved still.

On using the type of the instance, this was actually my first idea. The issue I found, is that it is not mandatory to provide type in the validation. Example :

{"type" : "string" , "pattern" : "^(foo|bar)$"}
works as well as
{"pattern" : "^(foo|bar)$"}

And, in the second instance type is not populated.

I don't know how TypeChecker works, I guess there is a mechanism which indicates what types the checks is what validation? And this could be used instead of hardcoding the values. And maybe this can be determined to check what type is the validator.

I'm quite new to the code there, so maybe I've made a mistake and not looked into the right property of Error, let me know if this is the case, or maybe where to look in order to find the type of the validator.

Regarding the const and enum validation, I didn't find any other way than to check the types of the values that are compared, and the type of the instance. Since those will be set by the user.

Thanks for your help, and looking forward to your advice.

[Julian]

I have added some tests so you can maybe better see what is the purpose of the PR.

Amazing, will have a look, appreciated!

On using the type of the instance, this was actually my first idea. The issue I found, is that it is not mandatory to provide type in the validation.

I think schemas without type should be beyond the current scope -- the scope of #728 should essentially be limited exactly to cases where type is present, and matches or doesn't match the branch taken.

It's true that there are even trickier things that can be done, but they're a lot more difficult, especially to design an interface that works in these additional scenarios where someone has customized what type string means, or has introduced additional validators.

(Which yes is exactly what TypeChecker helps do -- it says which python types a JSON Schema type corresponds to).

I'll have a closer look at this but yeah hopefully if we limit scope for a first PR it makes things significantly easier?

[miquelougoogle]

Hi,
Sorry for the delay, it took me a while to get into this.
I've done as you suggested, using 'type' property that's within the error.schema to determine the type of the rule.
I've left the const and enum logic unchanged from last commits.

I still think that using a map between the validator and which type of object it's validating would achieve more consistent results, for when type isn't provided. But this would mean refactoring all the _validators.py checks like those :

if not validator.is_type(instance, "object"):

To something like,

if not validator.is_type(instance, ACCEPTED_TYPE['this_function_name']):

And then using ACCEPTED_TYPE to find what type is the error, and see if it is the same type as the instance. Still a bit of an ugly implementation, but maybe good to dissociate the type check from the validator. Something to keep in mind for the future maybe.

Let me know what you think, and if you think the def get_(schema|instance)_type(value) should be moved elsewhere.