-
Notifications
You must be signed in to change notification settings - Fork 162
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 "--param-type" option to ros2param list. #572
add "--param-type" option to ros2param list. #572
Conversation
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.
A few things to change in here, but I like the idea.
could you check once again? |
Signed-off-by: Tomoya.Fujita <[email protected]>
Signed-off-by: Tomoya.Fujita <[email protected]>
Use a map so we don't have to continually do a search in the inner loop. Signed-off-by: Chris Lalancette <[email protected]>
6d077e3
to
174dd2f
Compare
@fujitatomoya FYI, I rebased this code and did a few more fixups to make the type lookup even faster. |
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.
One thing that would be really nice to do here would be to add some "cli" tests for the list
verb, like we have for dump
in https://github.com/ros2/ros2cli/blob/master/ros2param/test/test_verb_dump.py . I don't think it would be too hard to add, and would give us a bit more verification of this work. @fujitatomoya do you have some time to look into that?
Never mind. I missed that we had #389 already open, and apparently it is harder than I think since we never solved it there. I'm going to approve this and run CI on it. |
@claireyywang @mabelzhang I'd appreciate a second look here since I did some of the modifications myself. |
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.
LGTM
All right, we have approvals, green CI. I'm going to go ahead and merge, thanks @fujitatomoya ! |
thanks 👍 this is better! |
address #569
Signed-off-by: Tomoya.Fujita [email protected]