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.

Sign up for GitHub

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

Jump to bottom

Support passing parameters as a dictionary #138

Merged
merged 17 commits into from
Sep 21, 2018
Merged

Support passing parameters as a dictionary #138

merged 17 commits into from
Sep 21, 2018

Conversation

dhood
Copy link
Member

@dhood dhood commented Sep 14, 2018 •
edited
Loading

Closes #117
Requires ros2/rcl#299

The dictionary can be nested (to specify different groups of parameters) and can have substitutions at each level.

A yaml file is written following this structure. It always specifies the namespace and node name (requires ros2/rcl#299). Scoped out: deleting the file(s).

Design decisions:

  1. Typically "iterables" of substitutions will be concatenated into a single string. However, a user may have been trying to specify a parameter array with each element a substitution. To distinguish these two cases, only Tuples of substitutions will be concatenated into a string, not any Iterable. Lists of substitutions will be treated as a parameter array.
  2. Parameter files only work if the node name is specified, so if a parameter dict is passed, the node name must be specified also (https://github.com/ros2/launch/issues/139).

@dhood dhood added the in progress Actively being worked on (Kanban column) label Sep 14, 2018
@dhood dhood self-assigned this Sep 14, 2018
@dhood
Copy link
Member Author

dhood commented Sep 15, 2018 •
edited
Loading

Putting this in review thanks!

I've updated the PR description.
(linter failures)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@dhood dhood added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Sep 15, 2018
wjwwood
wjwwood reviewed Sep 17, 2018
View reviewed changes
launch_ros/launch_ros/actions/node.py
def _create_params_file_from_dict(self, context, params):
with NamedTemporaryFile(mode='w', prefix='launch_params_', delete=False) as h:
param_file_path = h.name
# TODO(dhood): clean up generated parameter files.
Copy link
Member

Choose a reason for hiding this comment

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

Is the intention to merge this and follow up with a pull request to clean up the files or do you think we can do without it?

I know we talked about it before and I said it would be ok to not do that for now, but now I'm thinking we should really clean up after ourselves. What are your thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that it's important also, was planning to leave it as followup to simplify reviewing of this PR. The tests are relying on the files being around atm so was going to also have an option to disable the cleanup.

@dhood
Copy link
Member Author

dhood commented Sep 18, 2018 •
edited
Loading

I'm gonna wait for ros2/rcl#299 before I merge this so that it works for nodes in the root namespace. Will move this out of the review column.

linters are good now: Build Status and xenial is OK too: Build Status

@dhood dhood merged commit ba77b77 into master Sep 21, 2018
@dhood dhood deleted the pass_parameters_dict branch September 21, 2018 21:51
@dhood dhood removed the in review Waiting for review (Kanban column) label Sep 21, 2018
@mkhansenbot mkhansenbot mentioned this pull request Oct 9, 2018
Closed
@dhood dhood mentioned this pull request Mar 22, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wjwwood wjwwood wjwwood approved these changes

Assignees

@dhood dhood

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@dhood @wjwwood