Skip to content
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

Add PositionableParam #278

Merged
merged 3 commits into from
Mar 7, 2024
Merged

Conversation

andlaus
Copy link
Collaborator

@andlaus andlaus commented Mar 5, 2024

This is the base class of all parameters for which a position within the PDU can be defined. Interestingly, the XSD does not seem to feature any parameters that are not positionable, but let's reflect the class hierarchy implied by the ODX specification...

Andreas Lauser <andreas.lauser@mercedes-benz.com>, on behalf of MBition GmbH.
Provider Information

@andlaus andlaus requested a review from kayoub5 March 5, 2024 08:00
@kayoub5
Copy link
Collaborator

kayoub5 commented Mar 5, 2024

I simply do not see the benefit of this PR, if non-positioned parameters simply don't exist, why introduce the complexity of a non-necessary abstraction layer?

@andlaus
Copy link
Collaborator Author

andlaus commented Mar 5, 2024

I simply do not see the benefit of this PR, if non-positioned parameters simply don't exist, why introduce the complexity of a non-necessary abstraction layer?

mainly to make it easier to look at the XSD and find the corresponding odxtools code. But I agree, we should probably solve this "by documentation" (i.e., add a comment that Parameter implements the POSITIONABLE-PARAM XSD group), and/or simply rename Parameter to PositionableParameter?

@kayoub5
Copy link
Collaborator

kayoub5 commented Mar 5, 2024

I simply do not see the benefit of this PR, if non-positioned parameters simply don't exist, why introduce the complexity of a non-necessary abstraction layer?

mainly to make it easier to look at the XSD and find the corresponding odxtools code. But I agree, we should probably solve this "by documentation" (i.e., add a comment that Parameter implements the POSITIONABLE-PARAM XSD group), and/or simply rename Parameter to PositionableParameter?

A documentation change should be more than enough

the `Parameter` class corresponds to the `POSITIONABLE-PARAM` type of
the ODX specification, not to `PARAM`. This is because the
specification does not define any non-positionable parameters.

Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io>
Signed-off-by: Christian Hackenbeck <christian.hackenbeck@mbition.io>
@andlaus
Copy link
Collaborator Author

andlaus commented Mar 6, 2024

okay. I moved all the "meat" of this PR into Parameter. (I'd like to keep the centralized position handling for the decoding routines.)

@andlaus andlaus force-pushed the positionable_param branch 2 times, most recently from a05a388 to 43a13ef Compare March 6, 2024 10:15
this avoids mistakes and simplifies the code. unfortunately, the
"leaf" parameter classes must do their decoding using the
`_decode_positioned_from_pdu()` method instead of
`decode_from_pdu()`. To avoid mistakes, I've marked
`Parameter.decode_from_pdu()` method as `@final` so that type checkers
can complain if it is overridden.

Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io>
Signed-off-by: Christian Hackenbeck <christian.hackenbeck@mbition.io>
Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io>
Signed-off-by: Christian Hackenbeck <christian.hackenbeck@mbition.io>
the next byte position after the extracted parameter
"""
pass
def _decode_positioned_from_pdu(self, decode_state: DecodeState) -> ParameterValue:
Copy link
Collaborator

@kayoub5 kayoub5 Mar 6, 2024

Choose a reason for hiding this comment

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

Maybe use @abstractmethod?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Other than this, LGTM

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

foreshadowing: abstract base classes prevent using the from_et() pattern for class hierarchies because for this pattern, base classes must be instantiated in the .from_et() constructor of child classes...

@andlaus andlaus merged commit cda6e5e into mercedes-benz:main Mar 7, 2024
7 checks passed
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.

None yet

2 participants