-
Notifications
You must be signed in to change notification settings - Fork 13
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
Do not use jsonschema.best_match because it obscures the errors #35
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test? I think just making sure an error has the correct types in its string form is fine.
hologram/__init__.py
Outdated
@@ -23,6 +23,7 @@ | |||
|
|||
from dateutil.parser import parse | |||
import jsonschema | |||
import pprint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is used!
I think this is an improvement, we should merge it and then see if we can do better later. Here's an example where this is definitely worse behavior in some ways: from typing import Union, Any
import dataclasses, hologram
@dataclasses.dataclass
class Bar(hologram.JsonSchemaMixin):
asdf: Union[str, Dict[str, Union[bool, str]]] = ''
Bar().from_dict({'asdf': {'key': 1}}) Results in this:
Which, while definitely correct, is very much not as correct as the old message:
I think this is the right change - I would rather have too much information than too little, as we currently do. But I also think we must be able to do better by picking some fancy relevance function for |
4b198f0
to
0bd7b91
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like mypy is mad about iter_errors
maybe not being an iterable. I'm sure we could teach mypy about jsonschema.iter_errors
, but my suggestion is probably easier and shouldn't break anything.
hologram/__init__.py
Outdated
@@ -956,6 +956,6 @@ def _get_field_type_name(field_type: Any) -> str: | |||
def validate(cls, data: Any): | |||
schema = _validate_schema(cls) | |||
validator = jsonschema.Draft7Validator(schema) | |||
error = jsonschema.exceptions.best_match(validator.iter_errors(data)) | |||
error = next(validator.iter_errors(data), None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why mypy is upset about this, but I think you want next(iter(validator.iter_errors(data)), None)
here.
0bd7b91
to
79bb73f
Compare
The 'best_match' code in jsonschema was switching to the "first" error encountered and so the exception didn't have all of the information that we want. This is for issue #2700 in dbt.