Repository navigation
Conversation
Follow-up to mercedes-benz#499, which covered the four places that require a simple DOP. Five more messages in the load path name neither the element they are about nor what was found instead: ecumemconnector.py "Invalid type of referenced object" commrelation.py "No DIAG-COMM referenced by COMM-RELATION" matchingparameter.py "Output parameter must not be left unspecified" subcomponentparamconnector.py (x2) "Currently, only SNREFS are supported for ..." The first says nothing at all, and in commrelation.py the message two lines below it already names the offending type, so the two read very differently for what is the same kind of problem. Each now names the element and, where the file offers something to compare against, what was found: the resolved type for a LAYER-REF, the tag actually used in a *-PARAM-IF-REFS list, the DIAG-COMM a MATCHING-PARAMETER belongs to. One further site was left alone: dynenddopref.py reports a missing DYN-END-DOP-REF tag, and at that point the element does not exist, so there is nothing to name. No behaviour changes; only the text of the diagnostics. Signed-off-by: George Vasiliades <1221374+Poseidonas@users.noreply.github.com>
CI caught this on the linting job. I had run mypy against odxtools/ only, which is not what the pipeline does - it runs `mypy .` and so covers tests/ as well. Running the same command locally now. Signed-off-by: George Vasiliades <1221374+Poseidonas@users.noreply.github.com>
| if not isinstance(x, (BaseVariant, EcuVariant)): | ||
| odxraise("Invalid type of referenced object") | ||
| odxraise(f"LAYER-REF '{ref.ref_id}' of ECU-MEM-CONNECTOR " | ||
| f"'{self.short_name}' references a {type(x).__name__}, but a " |
There was a problem hiding this comment.
in this context using x.__class__.__name__ works better than type(x).__name__ because it works fine with weakref.proxy objects...
There was a problem hiding this comment.
Done, and thank you for pointing at it twice over — this is the second time the same thing has caught me, so it has landed properly now.
Confirmed why it matters here rather than taking it on trust: layer_refs are resolved through odxlinks.resolve(), and OdxLinkDatabase carries use_weakrefs, so x really can arrive as a proxy.
type(proxy).__name__ -> weakproxy
proxy.__class__.__name__ -> BaseVariant
Which is precisely the case the message exists to describe, so it would have gone blank exactly when it was needed.
One thing I have deliberately left alone: the same pattern is in about thirty other files, including two lines in commrelation.py and subcomponentparamconnector.py that this PR touches but did not introduce. That felt like a separate change rather than something to fold in here without asking. Happy to open it as its own PR if it is worth having.
| @@ -0,0 +1,54 @@ | |||
| # SPDX-License-Identifier: MIT | |||
There was a problem hiding this comment.
I think this it is better not to test the specifics of the exception messages (i.e., remove the unit tests file)
There was a problem hiding this comment.
Removed. You are right, and the reasoning applies more widely than I had it: asserting on the wording pins the exact thing the PR is trying to leave room to improve. The next person to reword a message would have had to edit a test to say the same thing twice.
mypy ., ruff check . and the suite are clean after both changes: 325 files, 159 passed, 1 skipped.
`type(x).__name__` reads as `weakproxy` once the object comes back from a resolver built with use_weakrefs, which is the one case where the message is supposed to say what was actually referenced. The tests asserted on the wording of the exceptions, which pins text that should stay free to improve. Signed-off-by: George Vasiliades <1221374+Poseidonas@users.noreply.github.com>
ee57b2e to
b8850c7
Compare
|
Both points are addressed and the sign-off I had dropped is back, so DCO is green again.
Nothing in the file uses it any more, so ruff 0.16.4 reports F401. Checking both trees with the same ruff the CI installs: The pull request build merges into I have deliberately not touched it here — it is unrelated to this change and folding it in would make the diff say two things at once. Happy to send it as a one-line PR of its own if that is useful, or leave it with you. |
|
thanks for the patch, merging.
go wild :) |
Follow-up to #499. That one covered the four places which require a simple DOP; while looking at them I noted five more messages in the load path that name neither the element they are about nor what was found instead.
ecumemconnector.pyInvalid type of referenced objectcommrelation.pyNo DIAG-COMM referenced by COMM-RELATIONmatchingparameter.pyOutput parameter must not be left unspecifiedsubcomponentparamconnector.py(×2)Currently, only SNREFS are supported for …-PARAM-IF-REFSThe first one is the reason I bothered: on a database with several ECU-MEM-CONNECTORs it says nothing at all. And in
commrelation.pythe message two lines below already readsDIAG-VARIABLE references non-service {type} diagnostic communication, so two adjacent checks on the same object report very differently.Each message now names the element, and where the file gives something to compare against, what was found:
One site deliberately left alone
dynenddopref.py:32reports a missing mandatoryDYN-END-DOP-REFtag. The element does not exist at that point, so there is nothing to name — adding the parent would mean threading it throughfrom_et, which seemed disproportionate for one message.Verification
yapf,ruffandmypyclean; suite 159 → 161. The two new tests intests/test_load_error_context.pyboth fail onmainand pass with the change — theSUB-COMPONENT-PARAM-CONNECTORone because the message carried no name at all, so there was nothing to assert on.Behaviour is unchanged; only the text of the diagnostics. Same caveat as last time on wording — happy to adjust if you would phrase any of these differently.