Refactor decoding, part 4 #267
Conversation
for whatever reason, mypy started to complain recently. Maybe this has something to do with the recent switch to `InquirerPy`... Signed-off-by: Andreas Lauser <[email protected]> Signed-off-by: Florian Jost <[email protected]>
i.e., if they are non-byte aligned their value will no longer feature garbage at the beginning and/or at the end. Besides this, their values are now integers, which is more intuitive IMO. Signed-off-by: Andreas Lauser <[email protected]> Signed-off-by: Florian Jost <[email protected]>
Signed-off-by: Andreas Lauser <[email protected]> Signed-off-by: Florian Jost <[email protected]>
…_value()` Signed-off-by: Andreas Lauser <[email protected]> Signed-off-by: Florian Jost <[email protected]>
| @@ -99,7 +99,7 @@ def prompt_single_parameter_value(parameter: Parameter) -> Optional[AtomicOdxTyp | |||
| return None | |||
| elif parameter.physical_type.base_data_type is not None: | |||
| return _convert_string_to_odx_type( | |||
| answer.get(parameter.short_name), parameter.physical_type.base_data_type) | |||
| cast(str, answer.get(parameter.short_name)), parameter.physical_type.base_data_type) | |||
There was a problem hiding this comment.
| cast(str, answer.get(parameter.short_name)), parameter.physical_type.base_data_type) | |
| str(answer.get(parameter.short_name)), parameter.physical_type.base_data_type) |
There was a problem hiding this comment.
not sure: without the cast mypy has the following complaint:
odxtools/cli/browse.py:102: error: Argument 1 to "_convert_string_to_odx_type" has incompatible type "str | bool | list[Any] | None"; expected "str" [arg-type]
I guess it is better to make it shut up and deal with the exception if something goes wrong anyway instead of making it "work" unconditionally?
odxtools/diagcodedtype.py
Outdated
| @@ -70,7 +70,7 @@ def is_highlow_byte_order(self) -> bool: | |||
| return self.is_highlow_byte_order_raw in [None, True] | |||
|
|
|||
| @staticmethod | |||
| def _extract_internal_value( | |||
| def _extract_atomic_value( | |||
There was a problem hiding this comment.
I wanted a different name: there are/were so many methods with "internal" in their name that I get confused quite easily. (Also, I think that the new value better conveys the fact that this method returns an AtomicOdxType object...)
| byte_length = (8 * self.byte_length + bit_position + 7) // 8 | ||
| result = decode_state.coded_message[byte_position:byte_position + byte_length] | ||
| decode_state.cursor_byte_position += byte_length | ||
| result, decode_state.cursor_byte_position = DiagCodedType._extract_atomic_value( |
There was a problem hiding this comment.
Is it just me, or a function that takes decode_state directly would be easier to understand and re-use?
There was a problem hiding this comment.
this is the lowest level method which IMO should be usable without a decode_state and even without a DiagCodedType object. (also, DCTs already have a decode_from_pdu() method...)
There was a problem hiding this comment.
then why it is in the DiagCodedType class?
There was a problem hiding this comment.
because usually (all occasions instead of this one), it is called from there.
There was a problem hiding this comment.
(_extract_atomic_value() deals with retrieving a non-byte-aligned object from a blob of binary data, while decode_from_pdu() also deals with the "book keeping" of updating decode_state.)
There was a problem hiding this comment.
maybe put the function in the decode state itself?
There was a problem hiding this comment.
okay, did that. Thanks for the suggestion...
andlaus
force-pushed
the
refactor_decoding4
branch
from
cc89a58
to
c843a69
Compare
andlaus
force-pushed
the
refactor_decoding4
branch
from
c843a69
to
59324c3
Compare
thanks to [at]kayoub5 for the proposal. Signed-off-by: Andreas Lauser <[email protected]> Signed-off-by: Florian Jost <[email protected]>
This PR is the final part of the decode code refactoring miniseries. It contains the "leftovers" which I've encountered and fixed while implementing the rest of the series and which require the API changes of the rest of the series to be present.
Andreas Lauser <[email protected]>, on behalf of MBition GmbH.
Provider Information