Skip to content

DataObjectProperty.decode_from_pdu: support returning the default value when the internal value is invalid#389

Merged
kayoub5 merged 6 commits intomercedes-benz:mainfrom
nada-ben-ali:NaBe/return_default_if_internal_value_is_invalid
Feb 19, 2025
Merged

DataObjectProperty.decode_from_pdu: support returning the default value when the internal value is invalid#389
kayoub5 merged 6 commits intomercedes-benz:mainfrom
nada-ben-ali:NaBe/return_default_if_internal_value_is_invalid

Conversation

@nada-ben-ali
Copy link
Collaborator

This merge request adds support for returning the default value when the internal value is invalid. If the internal value is not defined, the method will check for a defined default value and return it; otherwise, it will raise an exception.

odxtools version: 9.2.0

@andlaus
Copy link
Member

andlaus commented Feb 18, 2025

note: if you want to get rid of these pesky complaints about the coding style, just install yapf and isort via pip and run the reformatSource.sh script in the top level directory of your working copy...

return str(default_value)

# TODO: How to prevent this?
raise DecodeError(f"DOP {self.short_name} could not convert the coded value "
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andlaus what do you think about using str(internal_value) when not in strict mode as final fallback ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really know: my hunch is that None is better because if the callling code encounters an unexpected type like str instead of int it almost certainly goes belly-up anyway and it is probably mentally a bit easier to associate None with "did not work". On the other hand, the internal value of the parameter would be lost in the warnings...

Comment on lines +139 to +144

internal_to_phys = self.compu_method.compu_internal_to_phys
default_value = internal_to_phys.compu_default_value if internal_to_phys else None

if default_value is not None:
return str(default_value)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you have a reference to the spec for this? It still seems fishy to me to fall back to a default value when receiving an undecodable value. (encoding a PDU where the parameter in question is left unspecified is a different story IMO.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Section 7.3.6.6.7

The optional COMPU-DEFAULT-VALUE can be used to define the physical value if the internal value does not lie in any given interval.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, thanks. That's a build-in footgun IMO, but...

Copy link
Member

@andlaus andlaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merge at will

@kayoub5 kayoub5 merged commit 3b369b5 into mercedes-benz:main Feb 19, 2025
7 checks passed
@nada-ben-ali
Copy link
Collaborator Author

@andlaus is it possible to get a new release today? Thank you!

@nada-ben-ali nada-ben-ali mentioned this pull request Feb 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants