[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

Add --with-comments option to dump #283

Merged
merged 3 commits into from
Mar 2, 2021

Conversation

simontegelid
Copy link
Collaborator

Add an option to include signal comments and units in the database dump output.

@coveralls
Copy link
coveralls commented Feb 25, 2021

Coverage Status

Coverage increased (+0.02%) to 96.588% when pulling d8fc22b on simontegelid:dump_comments into 7786a1d on eerimoq:master.

Copy link
Member
@andlaus andlaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good. If the potentially problematic module imports in database.py can be avoided, I'm all for merging this!

cantools/subparsers/dump.py Outdated Show resolved Hide resolved
cantools/database/utils.py Outdated Show resolved Hide resolved
cantools/database/can/message.py Outdated Show resolved Hide resolved
cantools/database/can/message.py Outdated Show resolved Hide resolved
if with_comments:
com = []
if siginst.comment:
com.append(siginst.comment)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed this: the ARXML file format supports multi-lingual comments, so the canonical way to access the comments is the .comments attribute of messages/signals. (which is a language string -> comment string dictionary. .comment is just the first entry of this dictionary.) If you don't consider it worthwhile to support such an exotic feature, I think this PR should go in as-is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like an additional PR to support that. I will keep it in the back of my head though :) Thanks for your review effort!

@eerimoq eerimoq merged commit fa02f5b into cantools:master Mar 2, 2021
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