Enhancing by_relevance / best_match (PR for issue 728)

[miquelougoogle]

No description provided.

[Julian]

So! Thanks again for trying to tackle this.

I still want to go for something a lot simpler than the code here. Maybe this will help as a start -- I think the changes to this file and this function probably should be just something like:

        same_type = False
        if "type" in error.schema:
            same_type = error._type_checker.is_type(error.instance, error.schema["type"])
        validator = error.validator
        return (
            -len(error.path),
            error.validator not in weak,
            error.validator in strong,
            not same_type,
        )

The missing piece there is that the type checker that's used is not in fact carried onto Errors at the minute, but that should be 1-2 lines to add.

If you want to see what that does, you can try instead starting with:

        same_type = False
        if "type" in error.schema:
            same_type = _types.draft7_type_checker.is_type(error.instance, error.schema["type"])
        validator = error.validator
        return (
            -len(error.path),
            error.validator not in weak,
            error.validator in strong,
            not same_type,
        )

And looking to see what it does to your test cases. From here it looks like it passes your first new one (which was the one that #728 was mainly targeting).

It fails the other two, but I'm not 100% convinced on the other two personally. If we do want to support them though, which yeah we should think about, there are other ways to handle them -- e.g. perhaps errors from non-container types (simple value types, e.g. strings, bools, etc.) should prioritize enum and const validators.

Hopefully the above is a breadcrumb to move this further a bit?

Let me know either way, and probably also worth having a look at CI to make sure it passes with whatever change, it'll be needed before we merge.

Appreciated again for staying on top of this!

[Julian]

Left a comment with some next steps!

[Julian]

@miquelougoogle did the comments I left make sense here? Just making sure you saw them.

[miquelougoogle]

Hi Julian,
Sorry for my lack of answers lately, I haven't forgotten this project at all, and meant to continue working on it. Your comments make sense to me, and I agree with your proposed solution.
Carrying the type to the Error object, so it can be checked in the by_relevance method.

I am not sure of the consequences of prioritizing const and enum over other types of error. (If I understood you right, which I am not sure I did)

Given the following example :
Schema :

{
    "anyOf": [
        {"type" : "string", "pattern" : "-?\\d+"},
        {"const" : "NA"}
    ]
}

Instance :

42

(This is provided as a number, not as a string, as the example schema specifies)

The error reported if const / enum would be prioritized without looking at the type contained, would be something like :

Error awaiting 'NA' got 42

Instead of the error telling the user that the type is incorrect.

I think enum and const should only be prioritized over type error if the type provided in the instance is correct. As getting an error letting you know that the type provided is incorrect doesn't make sense, if you actually have provided the correct type, but the wrong value. In the example, let's say that the user provides the value

"NULL"

In that case, the error that makes the most sense is that it is not "NA", though the user may get the pattern issue instead of the const one in the example. But getting an error saying that the type is incorrect is I believe inadequate.

So I believe knowing if the instance has the same_type of the const / enum.

Another argument, would be to keep the behaviour of {"type" : "string", "pattern" : "NA"} the same as {"const" : "NA"}

Thanks for taking the time,
Ben

[miquelougoogle]

Hi Julian,

I won't have the time to work on this cl, and it looks like I won't have any for the near future. I'll close the PR until I'm ready to spend more time on it.

Thanks for your comprehension.
Ben

[Julian]

No worries, definitely understandable. Hope all's well and thanks for your efforts.

…

On Thu, Dec 3, 2020, 05:04 miquelougoogle ***@***.***> wrote: Closed #753 <#753>. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#753 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACQQXQIQGRJIESNSEM4Z2TSS5PBFANCNFSM4S4SOXGQ> .