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

Implement get_parameter_or_set_default. #551

Merged
merged 3 commits into from
Sep 20, 2018

Conversation

clalancette
Copy link
Contributor

@clalancette clalancette commented Sep 6, 2018

This is syntactic sugar to allow the user to get a parameter.
If the parameter is already set on the node, it gets the value
of the parameter. If it is not set, then it gets the alternative
value and sets it on the node, ensuring that it exists.

I personally think that the name makes some sense, but I'm definitely open to other ideas for the API. If approved and merged, this will be used in ros2/teleop_twist_joy#8 . Test for this new functionality is in ros2/system_tests#296

@clalancette clalancette added the in progress Actively being worked on (Kanban column) label Sep 6, 2018
@clalancette
Copy link
Contributor Author

clalancette commented Sep 6, 2018

CI:

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

(note that the test failures are ros2/rmw_fastrtps#226, so unrelated to this PR)

@clalancette clalancette added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Sep 6, 2018
Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

Just 1 nitpick.

Also, it looks like this method won't be available to a lifecycle node, but I don't think anything needs to be done since get_parameter_or() is the same.

/// Get the parameter value; if not set, set the "alternative value" and store it in the node.
/**
* If the parameter is set, then the "value" argument is assigned the value
* in the parameter. If the parameter is not set, then the "value" argument
Copy link
Contributor

Choose a reason for hiding this comment

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

nit 1 sentence per line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed now.

@@ -250,6 +250,22 @@ Node::get_parameter_or(
return got_parameter;
}

template<typename ParameterT>
void
Node::get_parameter_or_set_default(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this could be shortened to get_parameter_or_set, but that's subjective of course.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this could be shortened to get_parameter_or_set, but that's subjective of course.

True. I don't have a strong opinion, though I typically prefer a bit more verbose. I'm going to leave it as get_parameter_or_set_default unless someone voices a strong opinion for something else.

(and thanks for the review!)

Copy link
Member

@dirk-thomas dirk-thomas Sep 13, 2018

Choose a reason for hiding this comment

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

Since it isn't setting the default value (since a parameter in the server side doesn't have a default value) I would second the suggestion from @sloretz.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All right, I renamed to get_parameter_or_set in 0bd546d.

This is syntactic sugar to allow the user to get a parameter.
If the parameter is already set on the node, it gets the value
of the parameter.  If it is not set, then it gets the alternative
value and sets it on the node, ensuring that it exists.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Copy link
Contributor Author

New CI:

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

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Copy link
Contributor Author

Thanks for the review, merging.

@clalancette clalancette merged commit be8c05e into master Sep 20, 2018
@clalancette clalancette deleted the get-parameter-or-set-default branch September 20, 2018 13:21
@clalancette clalancette removed the in review Waiting for review (Kanban column) label Sep 20, 2018
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