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 support for parameter prefixes in ros2 param list #131

Merged
merged 2 commits into from Aug 9, 2018

Conversation

mikaelarguedas
Copy link
Member

No description provided.

@mikaelarguedas mikaelarguedas added the in progress Actively being worked on (Kanban column) label Aug 8, 2018
@mikaelarguedas
Copy link
Member Author

CI just for linters: Build Status

@mikaelarguedas mikaelarguedas added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Aug 8, 2018
@@ -46,6 +49,9 @@ def main(self, *, args): # noqa: D102
if args.node_name not in node_names:
return 'Node not found'
node_names = [args.node_name]
param_prefixes = []
if args.param_prefixes is not None:
param_prefixes = [args.param_prefixes]
Copy link
Member

Choose a reason for hiding this comment

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

Won't this always be a list of 1?

@@ -36,6 +36,9 @@ def add_arguments(self, parser, cli_name): # noqa: D102
parser.add_argument(
'--include-hidden-nodes', action='store_true',
help='Consider hidden nodes as well')
parser.add_argument(
'--param-prefixes', nargs='?',
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure nargs='?' is the right choice because it allows an empty --param-prefixes

$ ros2 param list --param-prefixes
listener:
  bar.baz
  foo
  foo.bar
  foo.bar.baz

Is the intent to allow multiple prefixes to be specified? If so, nargs='+' or action='append' might be better choices.

@mikaelarguedas
Copy link
Member Author

Thanks for the feedback, I simplified the logic in c67f9d8 and it should now support multiple prefixes

@mikaelarguedas mikaelarguedas merged commit 3aea7c4 into master Aug 9, 2018
@mikaelarguedas mikaelarguedas deleted the param_list_prefixes branch August 9, 2018 00:30
@mikaelarguedas mikaelarguedas removed the in review Waiting for review (Kanban column) label Aug 9, 2018
esteve pushed a commit to esteve/ros2cli that referenced this pull request Dec 16, 2022
esteve pushed a commit to esteve/ros2cli that referenced this pull request Dec 16, 2022
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

3 participants