Convert OdxLinkId.doc_fragments and OdxLinkRef.ref_docs to tuple #410

zariiii9003 · 2025-04-04T06:40:33Z

As suggested in #405.

I tried converting OdxLinkRef into a frozen dataclass, but subclasses assign attributes after instantiation.

zariiii9003 · 2025-04-04T06:54:32Z

@andlaus Unrelated: i think there are a lot of mypy issues hidden by the Noreturn in the odxraise method. It should be NoReturn | None and the TYPE_CHECKING should be removed.

andlaus · 2025-04-04T09:23:43Z

The Noreturn is on purpose: Semantically odxraise() should be considered identical to raise. The difference is that in non-strict mode it becomes a no-op, i.e. the reason why there is often code after the odxraise() call is to provide a sensible fallback for non-strict mode (which does not make any guarantees about undefined behavior or working in general, just that it "does its best"). So odxraise() should stay Noreturn in the main branch, but for development purposes, it I agree that could be temporarily changed to None to uncover the issues which you mentioned (I suppose that you will get a lot of mypy errors then...)

andlaus · 2025-04-04T09:28:30Z

I tried converting OdxLinkRef into a frozen dataclass, but subclasses assign attributes after instantiation.

IIRC, that was also the issue I stumbled over, ParentRef was particularly bad because it does quite a bit of reference resolution...

zariiii9003 · 2025-04-04T17:48:57Z

A few more changes after a bit of research. To my understanding there are two options: the doc fragments are either just the category, or the category+diag_layer.

i changed the type annotation of OdxDocContext.doc_fragments to reflect that. The category context is now completely defined in Database._process_xml_tree(). The DiagLayerContainer then creates a new context for its layers.

odxtools/comparamsubset.py

odxtools/diaglayercontainer.py

odxtools/diaglayers/diaglayerraw.py

odxtools/odxdoccontext.py

andlaus

looks pretty good. when the match statement is replaced and it is verified that ODX 2.0 does not regress (you might need to help out on that front @kayoub5) I think this should be merged...

odxtools/database.py

tests/test_variant_patterns.py

kayoub5 · 2025-04-05T12:26:16Z

@nada-ben-ali could you check that odx 2.0 compatibility is not broken with those changes ?

kayoub5 · 2025-04-05T16:17:46Z

odxtools/diaglayers/hierarchyelement.py

@@ -31,7 +31,7 @@
 from .hierarchyelementraw import HierarchyElementRaw

 if TYPE_CHECKING:
-    from .database import Database
+    from ..database import Database


mypy didn't catch this ?

@zariiii9003: how did you find this? if using tools, can/should we change the CI system to do the same?

I noticed red squiggly lines in my IDE. I also wonder why both ruff and mypy seem to ignore it

ok. Seems like you should continue to let your IDE look at odxtools. mine (emacs) is not sophisticated enough ;)

andlaus · 2025-04-07T12:24:40Z

sorry for the conflict due to #411. as I see it, the .doc_frags attribute should not be defaulted anyway...

andlaus · 2025-04-07T18:11:37Z

thanks for rebasing. as soon as @nada-ben-ali reports back, I'll merge this.

I don't know why this blew up my smoke testing schript only now... Signed-off-by: Andreas Lauser <[email protected]> Signed-off-by: Alexander Walz <[email protected]>

andlaus · 2025-04-10T10:24:06Z

For some strange reason, I ran into some DOCREF-related issue with this. I don't know why it only crept up with this PR, but I made a "meta pull request" for your PR here: zariiii9003#1 . would be great if you merged it...

fix DOCREF for PARENT-REFs using implicit document fragments

andlaus · 2025-04-10T13:00:21Z

@nada-ben-ali: were you able to test this with ODX 2.0? I think it is relatively unlikely that it regresses something on that front, and since I get a weird performance regression which goes away with this PR, I'd like to get it merged quickly...