-
-
Notifications
You must be signed in to change notification settings - Fork 555
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
Introduce new ARXML traversal functions #237
Introduce new ARXML traversal functions #237
Conversation
is is basically a temporary fix to keep the tests passing after the next patch.
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.
Hi!
Looks good, just a bunch of styling comments. I did not write comments for all styling issues. Try to apply similar styling fixes throughout the code.
Please use '
and not "
for strings. "
can be used if the string itself contains a '
.
Thanks!
The `follow_arxml_reference()` method follows an arbitrary relative or absolute ARXML reference to its target node (surprise!), the method `get_arxml_children()` returns the list of child nodes of a given set of elements that match a specification. (Note that the syntax of locations has been slightly extended compared to what was there before: if a tag name is preceeded by an '&', ARXML references are allowed, if is prefixed by '*', multiple nodes are possible. These qualifiers can also be combined.) Finally, there is the `get_unique_arxml_child()` convenience function which is just like `get_arxml_children()` but expects to match at most one child node, i.e., the returned object can be used as before. Note that the `follow_arxml_reference()` method is basically a generalized version of the old `SystemLoader.find()` method, whilst `get_arxml_children()` is an extended version of using ElementTree's `iterfind()` method on nodes. (It is more general, because it can use on multiple nodes as its base and it also can resolve ARXML references automatically.)
With this patch, ARXML files are *much* freer to use references and IMO even the python code's readability improves. Be aware that I'm not sure where exactly references are allowed by the AUTOSAR standard or how to obtain that information, so I've enabled them quite liberally. The rationale is that if AUTOSAR does not allow references in a given context but we support them anyway, well-formed files will still load fine. TODO (?): Adapt the EcuExtractLoader class.
in a few rare cases imposing a hard 80 character limit would completely screw up the indentation, so it is slighly larger there.
0098d76
to
e4c176f
Compare
Thanks for the review, I included the requested changes. I hope that I did not forget too much, because I was a bit "lost in git branches" for a while. I also have a few questions/remarks on why the original patches looked like they did:
|
Ok =) Are there more PR:s after this one? I'll fix the styling myself when everything has been merged as I think that's the quickest way forward. I'm too picky when it comes to styling :P |
yes, quite a few. I indent get what's in https://github.com/Daimler/cantools/tree/arxml_refactor fully upstream. This PR is basically just "ground clearance" work for the ARXML3 stuff...
Let's do it as you prefer, i'm fine if the whole process takes a while as long as the end result is good. I generally don't really care too much about coding style, so I will try to adapt for the remaining PRs, but I am prone to make mistakes, so please be a bit lenient with me ;) Small question: Shouldn't internal methods be prefixed by an underscore in python? |
Yeah, they probably should be prefixed with an underscore. In this case the user should never use this module directly as it is not documented in the official documentation. Any functions not part of the documentation is considered internal in this project. I guess that's not clear to anyone but me, but that's why it looks like it does. |
ok, I'll adapt the SystemLoader class in the next PR, then... |
Let's just continue merging. It would take forever to fix them all, and I can easily fix it once we are done. What's important now is that there are tests in place to ensure that the code works, and that future changes wont break legacy. Tests are hard for me to add later, styling is easy =) |
I just noticed that the "goal branch" on the Daimler github organization is not public for whatever reason. (Oops!) For your benefit, I thus also uploaded it to my personal account: https://github.com/andlaus/cantools/tree/arxml_refactor |
Ok, thanks. I'll have I look if I find the time, which is unlikely as I'm focusing on the Mys project atm. |
This PR continues the quest for ARXML-3 support, cf https://github.com/Daimler/cantools/tree/arxml_refactor
In this PR a set improved of traversal functions for ARXML documents is introduced and the existing code is changed to use them. The main advantage of them is that they make it very easy to follow ARXML references and to specifies colletions of XML nodes which might have ARXML references in their location.
Be aware that I'm not sure where exactly references are allowed by the AUTOSAR standard or how to obtain that information, so I've enabled them quite liberally. The rationale is that if AUTOSAR does not allow references in a given context but we support them anyway, well-formed files will still load fine.
Andreas Lauser andreas.lauser@mbition.io, Mercedes-Benz AG on behalf of MBition GmbH.
Imprint