refactor the initialization and reference resolution mechanism

[andlaus]

unfortunately, this PR firmly belongs into the "long and boring" category:

It cleans up the object initialization code in the sense that the API is now much more consistent; Now, all non-primitive objects which are part of a diagnostic layer provide a ._build_odxlinks() method to create a dictionary of all OdxLinkId mappings contained by this object. These dictionaries are then collected for the whole database and used to resolve ODXLINK references via the ._resolve_odxlinks(odxlinks) methods. Once this is done, the diagnostic layers deal with inheritance of objects, whereafter the short name references (SNREFs) are resolved via the ._resolve_snrefs(diag_layer) methods.

The initialization of objects which are not part of any DiagLayer is slightly different: The Database object has a refresh() method which deals with the full initializtion logic of all objects it contains, the DiagLayerContainer and DiagLayer classes have _build_odxlinks() and _finalize_init() methods. _finalize_init() deals with inheritance and SNREF resolution for all objects contained.

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

[kayoub5]

resolve does not accept None and return None?

[andlaus]

well, the purpose of these methods is to resolve the internally contained references of the object, i.e. it modifies the object (akin list.sort())

[kayoub5]

I am asking if the "if self.table_row_ref:" is really necessary

[andlaus]

ah okay, my bad. the signature of this method currently is def resolve(self, ref: OdxLinkRef, expected_type: Optional[Type[T]] = None) -> Any. So it currently does not accept None, but you're right: it might avoid a few case distinctions if it did. On the flipside, it might come with a few more implicit assumptions which one must consider while reading code that uses it. I suppose I have to meditate a bit about it before I can form an opinion...

[kayoub5]

Duplicate with lines 66-73

[andlaus]

for (probably) historic reasons, this file contains several classes. Line 66 is InputParam, this here is for NegOutputParam. This should be fixed in a future PR. (all files should be made to contain at most a single class; I also get annoyed by this quite frequently.)

[kayoub5]

so maybe those lines need to go to a parent Param class?

[andlaus]

that might be a possibility for a future PR. I'm not really sure if this a terribly good idea though, because it makes things quite a bit less explicit, so I'd like to have a separate PR to assess that...

[kayoub5]

Still don't see the reason why you split _resolve_odxlinks and _resolve_snrefs, I believe it was better as one function

[andlaus]

Still don't see the reason why you split _resolve_odxlinks and _resolve_snrefs, I believe it was better as one function

that's because the SNREFS can only be resolved once the respective namespace for the short names is complete, i.e. once the diaglayer has sorted out inheritance of the objects that are subject to value inheritance and that's only really possible after the ODXLINKs have been resolved. Maybe the spec can be interpreted differently, (i.e., SNREFS could be only allowed for locally defined objects), but then stuff like the NOT-INHERITED-* tags of PARENT-REF would not make sense...

Besides this the current code is pretty hacky and fragile, because the _resolve_references() signature is different for objects that need to resolve short name references compared to the ones which don't. this becomes even more fun once class inheritance gets involved: sometimes the base class uses the variant without the DiagLayer parameter and the derived class the one with it. This happens e.g., for EndOfPduField. tl;dr: object initialization currently is a mess!

[kayoub5]

Still don't see the reason why you split _resolve_odxlinks and _resolve_snrefs, I believe it was better as one function

that's because the SNREFS can only be resolved once the respective namespace for the short names is complete, i.e. once the diaglayer has sorted out inheritance of the objects that are subject to value inheritance and that's only really possible after the ODXLINKs have been resolved. Maybe the spec can be interpreted differently, (i.e., SNREFS could be only allowed for locally defined objects), but then stuff like the NOT-INHERITED-* tags of PARENT-REF would not make sense...

Besides this the current code is pretty hacky and fragile, because the _resolve_references() signature is different for objects that need to resolve short name references compared to the ones which don't. this becomes even more fun once class inheritance gets involved: sometimes the base class uses the variant without the DiagLayer parameter and the derived class the one with it. This happens e.g., for EndOfPduField. tl;dr: object initialization currently is a mess!

this means _resolve_references should receive a dict(short_name => object) instead of directly receiving diag layer, or class specific extra arguments

[andlaus]

this means _resolve_references should receive a dict(short_name => object) instead of directly receiving diag layer, or class specific extra arguments

the problem here is that the namespace for these short name references depends on the type of object which specifies the reference. e.g. the for the diag_comms, the list of relevant objects is diag_layer.diag_comms, for the dops it is diag_layer.data_object_props and so on. Since there can -- and likely will -- be naming conflicts between the lists, such a simple mechanism is unfortunately not possible. (I don't know yet how SNPATHREFS can be implemented, but it fortunately seems like these are extremely uncommon...)

[kayoub5]

this means _resolve_references should receive a dict(short_name => object) instead of directly receiving diag layer, or class specific extra arguments

the problem here is that the namespace for these short name references depends on the type of object which specifies the reference. e.g. the for the diag_comms, the list of relevant objects is diag_layer.diag_comms, for the dops it is diag_layer.data_object_props and so on. Since there can -- and likely will -- be naming conflicts between the lists, such a simple mechanism is unfortunately not possible. (I don't know yet how SNPATHREFS can be implemented, but it fortunately seems like these are extremely uncommon...)

That means a SNREF database similar to the OdxLinkDatabase, with a resolve(self, short_name: str, expected_type: Optional[Type[T]] = None) and the internal storage with a dict key = (name, type)
This would be similar to how the OdxLinkDatabase have a storage with dict key = (doc_frag, odx_id)

I am not asking for this change to be done in this PR (it's already big as it is), I am asking for the concept to be considered for future PRs

[andlaus]

I am not asking for this change to be done in this PR (it's already big as it is), I am asking for the concept to be considered for future PRs

I'd be very open for such considerations, I just have some trouble imagining how this would work. (thus, do not expect too much from my side in this direction ;))

[andlaus]

okay: I moved most unrelated stuff into #141. Hopefully, this makes this PR more focused (since not doing so would create quite a few merge conflicts, this PR is based on top of #141, so until #141 is merged, the diff here will stay unchanged...)

[andlaus]

@kayoub5: do you see any major issues with this?

[kayoub5]

@kayoub5: do you see any major issues with this?

Looks good, but you will need to bump the major version once you are done with all the refactoring

A lot of API changes that could break things for users.

[andlaus]

Looks good, but you will need to bump the major version once you are done with all the refactoring

alright, I'll do this. let's get this merged, thanks for your patience...