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

Refactor table classes #164

Merged
merged 4 commits into from
Jul 24, 2023
Merged

Conversation

andlaus
Copy link
Collaborator

@andlaus andlaus commented Jul 21, 2023

this PR refactors the table classes once more:

  • Table rows now get a reference to the table which contains them. This is required to resolve SNREFs in table rows.
  • In Tables, the table rows are now specified as a mixed list of ODXLINK references and table row objects. The approach is identical to the one for diagnostic communication objects for diagnostic layer. The reason for this is that the XML specification defines rows this way (as with the diagnostic services, table.table_rows provides a fully resolved view, while table.table_rows_raw is the table row specification as it is found in the XML)
  • The TableRow class is put to a separate file and TableBase is subsumed into its only user, Table.
  • The DiagLayer class does not hold the table objects directly anymore. instead, they are moved to the DiagDataDictionarySpec like in the XML.
  • various smaller cleanups and refactorings

Andreas Lauser <[email protected]>, on behalf of MBition GmbH.
Provider Information

Signed-off-by: Andreas Lauser <[email protected]>
Signed-off-by: Gerrit Ecke <[email protected]>
- all sub-tags mentioned in the XSD for TABLE and TABLE-ROW should now
  be internalized.
- table rows are now handled like the diagnostic communications in the
  diag layer: they are specified using a mixed list of table rows and
  links to table rows. The latter are resolved in the
  `._resolve_odxlinks()` method, and the `.table_rows` property
  provides a "resolved" view uppon these objects. As a consequence,
  instead of specifying table rows and references separately, a single
  parameter (`table_rows_raw`) is now used.
- The `TableBase` class has been removed since the only class which
  derived from it was `Table` and the XSD also does not specify an
  anologous element...

Signed-off-by: Andreas Lauser <[email protected]>
Signed-off-by: Gerrit Ecke <[email protected]>
that's where they are defined in the XML...

Signed-off-by: Andreas Lauser <[email protected]>
Signed-off-by: Gerrit Ecke <[email protected]>
@andlaus andlaus requested a review from kayoub5 July 21, 2023 07:50
@@ -313,11 +311,6 @@ def global_negative_responses(self) -> NamedItemList[Response]:
"""All global negative responses applicable to this DiagLayer"""
return self._global_negative_responses

@property
def tables(self) -> NamedItemList[Table]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

The diaglayer.tables property is gone, how is the user supposed to access tables now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

via diag_layer.diag_data_dictionary_spec.tables. You have a point though, I should add a deprecated method which does that. (fixed: bdc56ee )

@kayoub5
Copy link
Collaborator

kayoub5 commented Jul 22, 2023

I tried to test this PR with the PDX files I have, found out that #163 caused breaking changes

thanks!

Signed-off-by: Andreas Lauser <[email protected]>
Signed-off-by: Gerrit Ecke <[email protected]>
@andlaus andlaus mentioned this pull request Jul 22, 2023
@andlaus
Copy link
Collaborator Author

andlaus commented Jul 22, 2023

I tried to test this PR with the PDX files I have, found out that #163 caused breaking changes

right, fixed (hopefully). note that this is a gap in the unit test coverage. Unfortunately, it's not completely trivial to add a test for End-of-PDU encoding...

@andlaus
Copy link
Collaborator Author

andlaus commented Jul 24, 2023

@kayoub5: Do you any showstoppers with this or can it be merged?

@kayoub5
Copy link
Collaborator

kayoub5 commented Jul 24, 2023

@kayoub5: Do you any showstoppers with this or can it be merged?

No show stoppers, just make a release once merged

@andlaus
Copy link
Collaborator Author

andlaus commented Jul 24, 2023

No show stoppers, just make a release once merged

cool, thanks. Since the latest release did not come with the EndOfPduField regression, I don't think that this is critical and I would like to wait for my next PR (for en- and decoding TableStruct parameters) is merged...

@andlaus andlaus merged commit 081fa7e into mercedes-benz:main Jul 24, 2023
@andlaus andlaus deleted the refactor_table_classes branch December 7, 2023 13:29
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.

2 participants