feat: Add raise_on_errors parameter for strict parsing validation#73
feat: Add raise_on_errors parameter for strict parsing validation#73eduardiazf wants to merge 2 commits intofrequenz-floss:v0.x.xfrom
raise_on_errors parameter for strict parsing validation#73Conversation
9052e0b to
01f668d
Compare
There was a problem hiding this comment.
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.
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.
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.
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.
Thanks a lot, I think Luca's comments are reasonable, otherwise looks good to me.
36dcfbe to
6cbf873
Compare
Signed-off-by: eduardiazf <eduardiazf@gmail.com>
…hierarchy and move validation logic to client level Signed-off-by: eduardiazf <eduardiazf@gmail.com>
6cbf873 to
86debfc
Compare
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
Resources