-
Notifications
You must be signed in to change notification settings - Fork 911
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
[rosmsg] Rosmsg info implemented as alias of rosmsg show #941
[rosmsg] Rosmsg info implemented as alias of rosmsg show #941
Conversation
+1 for each time I accidentally typed |
This is joint work of Javier with @OmniSliver (credit where credit is due) as part of a course I am teaching at U. Chile. They have a blog about their work (in Spanish) here: https://javierdiazp.github.io/robotica2/ |
tools/rosmsg/src/rosmsg/__init__.py
Outdated
sys.exit(rosmsg_cmd_show(ext, full)) | ||
sys.exit(rosmsg_cmd_show(ext, full, 'show')) | ||
elif command == 'info': | ||
sys.exit(rosmsg_cmd_show(ext, full, 'info')) |
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.
Possible alternative:
if command in ('show', 'info'):
sys.exit(rosmsg_cmd_show(ext, full, command))
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.
@javierdiazp Can you please update the code with the suggestion from @mikepurvis
This should target |
It needs to be manually cherry-picked due to the other branch changes. I'll do this now. |
36c3340
to
8e54b87
Compare
@ros-pull-request-builder test this please @dirk-thomas This is failing because Jenkins is triggering the indigo/jade jobs (which fail to merge) rather than kinetic. I believe it will pass though; my inclination is to just merge now and roll it back if there's a problem with the post-merge CI build. |
@ros-pull-request-builder retest this please |
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.
Can you please create a related PR to add the completion for this new verb: https://github.com/ros/ros/blob/58acf513dfdc056f922666820ac24aa7d25ff521/tools/rosbash/rosbash#L882
tools/rosmsg/src/rosmsg/__init__.py
Outdated
sys.exit(rosmsg_cmd_show(ext, full)) | ||
sys.exit(rosmsg_cmd_show(ext, full, 'show')) | ||
elif command == 'info': | ||
sys.exit(rosmsg_cmd_show(ext, full, 'info')) |
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.
@javierdiazp Can you please update the code with the suggestion from @mikepurvis
tools/rosmsg/src/rosmsg/__init__.py
Outdated
@@ -712,7 +712,8 @@ def fullusage(mode): | |||
return """%(cmd)s is a command-line tool for displaying information about ROS %(type_)s types. | |||
|
|||
Commands: | |||
\t%(cmd)s show\tShow %(type_lower)s description | |||
\t%(cmd)s show, %(cmd)s info | |||
\t\t\tShow %(type_lower)s description |
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.
I would suggest keeping the existing line and adding a new line for the alias:
\t%(cmd)s show\tShow %(type_lower)s description
\t%(cmd)s info\tAlias for %(cmd)s show
I've added a commit which addresses the review feedback, and the completion verb PR is at ros/ros#138. |
Output looks good:
Thanks for the contribution, @javierdiazp! |
All the other ros* commands use "info" to display their details, except rosmsg, which uses "show". In order to maintain consistency, the alias "rosmsg info" of "rosmsg show" was implemented.