- 
                Notifications
    
You must be signed in to change notification settings  - Fork 0
 
feat: ✨ handle grouped errors under resource fields #175
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
base: main
Are you sure you want to change the base?
Conversation
| errors_in_group = _get_errors_in_group(schema_errors, parent_error) | ||
| schema_errors.remove(parent_error) | ||
| 
               | 
          ||
| field_type: str = parent_error.instance.get("type", "string") | 
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.
Defaulting to string if the type is not given. This is because the string-schema is the only one that doesn't require type.
| schema_index = FIELD_TYPES.index(field_type) | ||
| errors_for_other_types = _filter( | ||
| errors_in_group, | ||
| lambda error: f"fields/items/oneOf/{schema_index}/" not in error.schema_path, | 
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.
We can choose the sub-schema by filtering on the schema path. E.g., the schema path of the string-based schema is ...fields/items/oneOf/0/...
| str(files("check_datapackage.schemas").joinpath("data-package-2-0.json")) | ||
| ) | ||
| 
               | 
          ||
| FIELD_TYPES = [ | 
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.
We can extract this from the schema on the fly if that's better
| 
           If we merge in #177, I will align this with that  | 
    
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.
Nice! Only some small comments 
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.
Very small change to the error message
| if field_type not in FIELD_TYPES: | ||
| unknown_field_error = SchemaError( | ||
| message=( | ||
| "Unknown Data Package field type. Please use one of" | 
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.
| "Unknown Data Package field type. Please use one of" | |
| "The resource's schema's field property doesn't have the correct type. It should be one of" | 
Just to be explicitly clear what we're meaning. I think this is correct right?
Description
This PR adds a function for handling grouped errors under
$.resources[x].schema.fields[x].Here, the problem comes from the fact that each field type has its own sub-JSON-schema, and each one of these sub-schemas flags issues when the type of a field is not its own type. So, if a field has
type="number"and there is something wrong with the field, then the sub-schemas foryear,string, etc. will also flag issues. The goal is to flag issues only fornumber.Part of #15
Needs an in-depth review.
Checklist
just run-all