Skip to content

Conversation

@eduardiazf
Copy link
Contributor

@eduardiazf eduardiazf commented Jan 26, 2026

Summary

Add raise_on_errors: bool = False parameter to parsing functions. When True, raises ParsingError with major_issues, minor_issues, and raw_message attributes instead of logging.

Changes

  • Add ParsingError exception in exceptions.py
  • Add raise_on_errors parameter to:
    • electrical_component_proto()
    • component_connection_from_proto()
    • microgrid_from_proto()
    • AssetsApiClient methods

Signed-off-by: eduardiazf <eduardiazf@gmail.com>
@eduardiazf eduardiazf force-pushed the fix/parsing-component-connection-errors branch from 9052e0b to 01f668d Compare January 26, 2026 10:29
@eduardiazf eduardiazf added the cmd:skip-release-notes It is not necessary to update release notes for this PR label Jan 26, 2026
Copy link
Contributor

@llucax llucax left a 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.

Comment on lines +23 to +24
*,
raise_on_errors: bool = False,
Copy link
Contributor

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)
Copy link
Contributor

@llucax llucax Jan 26, 2026

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).

Comment on lines 155 to 158
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
]
Copy link
Contributor

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

Copy link
Contributor

@cwasicki cwasicki left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cmd:skip-release-notes It is not necessary to update release notes for this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants