[go: nahoru, domu]

Skip to content
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

Merged
merged 4 commits into from
Nov 25, 2020

Conversation

andlaus
Copy link
Member
@andlaus andlaus commented Nov 24, 2020

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

is is basically a temporary fix to keep the tests passing after the next patch.
@coveralls
Copy link
coveralls commented Nov 24, 2020

Coverage Status

Coverage increased (+0.1%) to 96.502% when pulling e4c176f on Daimler:arxml_new_traversal_functions into c2e63be on eerimoq:master.

Copy link
Collaborator
@eerimoq eerimoq left a 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!

cantools/database/can/formats/arxml.py Outdated Show resolved Hide resolved
cantools/database/can/formats/arxml.py Outdated Show resolved Hide resolved
cantools/database/can/formats/arxml.py Outdated Show resolved Hide resolved
cantools/database/can/formats/arxml.py Outdated Show resolved Hide resolved
cantools/database/can/formats/arxml.py Show resolved Hide resolved
cantools/database/can/formats/arxml.py Show resolved Hide resolved
cantools/database/can/formats/arxml.py Show resolved Hide resolved
cantools/database/can/formats/arxml.py Outdated Show resolved Hide resolved
cantools/database/can/formats/arxml.py Outdated Show resolved Hide resolved
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.
@andlaus andlaus force-pushed the arxml_new_traversal_functions branch from 0098d76 to e4c176f Compare November 25, 2020 13:00
@andlaus
Copy link
Member Author
andlaus commented Nov 25, 2020

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:

  • It seems that the retrieval of sub-nodes is not done consistently in master: sometimes a get_*() method is used, sometimes it seems to be find_*() or load_*() and sometimes the ElementTree is traversed directly
  • I normally prefer to avoid the opacity of relatively trivial functions (e.g., getter functions for internal data), in particular if these functions are called from exactly one location.
  • I'm probably too much of a C++ guy and have cobbled together too many shell scripts in my live, so I usually use normal quotes for strings ;). since the current master is somewhat inconsistent on this front as well (E.g. the EcuLoader class uses double quotes in places which you do not mention above), I thought it would be a good idea unify the style of strings to something that's compatible with most other languages.

@eerimoq
Copy link
Collaborator
eerimoq commented Nov 25, 2020

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

@andlaus
Copy link
Member Author
andlaus commented Nov 25, 2020

Are there more PR:s after this one?

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...

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

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?

@eerimoq
Copy link
Collaborator
eerimoq commented Nov 25, 2020

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.

@andlaus
Copy link
Member Author
andlaus commented Nov 25, 2020

ok, I'll adapt the SystemLoader class in the next PR, then...

@eerimoq
Copy link
Collaborator
eerimoq commented Nov 25, 2020

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 =)

@eerimoq eerimoq merged commit 386f51a into cantools:master Nov 25, 2020
@andlaus andlaus deleted the arxml_new_traversal_functions branch November 25, 2020 14:04
@andlaus
Copy link
Member Author
andlaus commented Nov 25, 2020

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

@eerimoq
Copy link
Collaborator
eerimoq commented Nov 25, 2020

Ok, thanks. I'll have I look if I find the time, which is unlikely as I'm focusing on the Mys project atm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants