-
Notifications
You must be signed in to change notification settings - Fork 7
feat: Add raise_on_errors parameter for strict parsing validation
#73
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: v0.x.x
Are you sure you want to change the base?
feat: Add raise_on_errors parameter for strict parsing validation
#73
Conversation
Signed-off-by: eduardiazf <eduardiazf@gmail.com>
9052e0b to
01f668d
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.
I wrote this having an exception hierarchy like the following in mind:
ValidationError <--+--- InvalidMicrogridError
|
+--- InvalidElectricalComponentError
|
+--- ValidationErrorGroup (inherits from ExceptionGroup too) <------ InvalidElectricalComponentErrorGroup
I know this could be a bit overkill for a first version, so for a first approach I think we could just have one simple ValidationError and raise that with the first issue we find.
What I think is more important is not to add the raise_on_errors to xxx_from_proto but instead using the existing xxx_from_proto_with_issues.
| *, | ||
| raise_on_errors: bool = False, |
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 wouldn't add this here, I would add this at the client level.
| ) | ||
|
|
||
| return microgrid_from_proto(response.microgrid) | ||
| return microgrid_from_proto(response.microgrid, raise_on_errors=raise_on_errors) |
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.
Do the error handling here instead:
if raise_on_errors:
major_issues = []
minor_issues = []
microgrid = microgrid_from_proto_with_issues(response.microgrid, major_issues, minor_issues)
# For now I would ignore minor_issues, I have the feeling we should only have
# "errors" in the future, this minor/minor distinction is not really useful
if major_issues:
raise InvalidMicrogridError(microgrid, response.microgrid, major_errors, minor_errors)
return microgrid
else:
microgrid_from_proto(response.microgrid, major_issues)EDIT: Updated example to only call xxx_with_issues() if `raise_on_errors is true.
I would use InvalidMicrogridError rather than ParseError as we are not really parsing anything here, protobuf messages are already parsed and have no problem at the protobuf-level, is just some validations we are doing that don't pass.
In the future we might even change the argument for something like on_validation_error=LOG|RAISE_ONCE|RAISE_ALL|IGNORE (for now I think we can keep it simple and leave the raise_on_errors).
| return [ | ||
| electrical_component_proto(component) for component in response.components | ||
| electrical_component_proto(component, raise_on_errors=raise_on_errors) | ||
| for component in response.components | ||
| ] |
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.
Same, but we could use a ExceptionGroup to raise many exceptions:
components = []
exceptions = []
for component_pb in response.components:
major_issues = [] # minor_issues...
component = electrical_component_from_proto_with_issues(component_pb, major_issues, ...)
if major_issues:
exceptions.append(
InvalidElectricalComponentError(component, component_pb, major_issues, ...)
)
else:
components.append(component)
if exceptions:
raise InvalidElectricalComponentErrorGroup(valid_components=components, exceptions)
return components
cwasicki
left a comment
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.
Thanks a lot, I think Luca's comments are reasonable, otherwise looks good to me.
Summary
Add
raise_on_errors: bool = Falseparameter to parsing functions. When True, raises ParsingError with major_issues, minor_issues, and raw_message attributes instead of logging.Changes