-
Notifications
You must be signed in to change notification settings - Fork 80
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
resolve snrefs for child items #373
base: main
Are you sure you want to change the base?
Conversation
@@ -91,6 +91,7 @@ def _resolve_odxlinks(self, odxlinks: OdxLinkDatabase) -> None: | |||
self._table_row = odxlinks.resolve(self.table_row_ref, TableRow) | |||
else: | |||
self._table_row = odxlinks.resolve(self.table_row_ref) | |||
self._table_row._resolve_odxlinks(odxlinks) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this line, the parser will raise exception on below line, said that the "table" of tablerow does not exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. That said, _resolve_odxlinks()
must not be called for objects that are the result of a reference resolution lest there be cyclic reference resolution hell. as an alternative to this patch line 94 should better be replaced by
self._table = odxlinks.resolve(self._table_row.table_ref, Table)
in this case, the .table
property below should also be simplified to
@property
def table(self) -> "Table":
return self._table
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you want you change the PR to only fix this issue (and remove the SNREF changes) or do you prefer if I do it?
for response in self._positive_responses: | ||
response._resolve_snrefs(context) | ||
for response in self._negative_responses: | ||
response._resolve_snrefs(context) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while this might fix the specific problem which you've encountered, this is pretty much a coincidence: this causes the referenced objects to be retargeted to basically the last ECU variant encountered during the reference resolution procedure. If you're looking at another diagnostic layer, e.g. the base variant, its snrefs will have been incorrectly resolved...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, u are correct. So with this change, I need to call _resolve_snrefs again before I try to visit other variants. Since the child variant would use parent objects (References) directly, I cannot find other solutions for this problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess #374 might make your life considerably easier?!
for issue #368, current solution is to resolve snrefs based on current variant context before trying to access it. To get the correct result, the snrefs of child items should be resolved when calling its parent's _resolve_snrefs. So I add code to do the work for some missing items