Skip to content

Fix documentation of Context class#2107

Merged
clalancette merged 1 commit intoros2:rollingfrom
traversaro:fix2106
Feb 16, 2023
Merged

Fix documentation of Context class#2107
clalancette merged 1 commit intoros2:rollingfrom
traversaro:fix2106

Conversation

@traversaro
Copy link
Copy Markdown
Contributor

@traversaro traversaro commented Feb 16, 2023

Fix #2106 . I also removed the reference to rclcpp::init as it does take in input a Context, I added instead the method to call to specify the context to use for a given node, please correct me if I was wrong on that.

As an alternative, we can also get rid of that sentence.

Comment on lines 68 to 69
* It is often used in conjunction with rclcpp::NodeOptions::context
* and rclcpp::shutdown.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

While this sentence is now technically true, I don't think it is particularly helpful. It doesn't tell the reader how this might be used.

Instead, what I think we should say here is something along the lines of:

Nodes may be attached to a particular context by constructing a Node with an rclcpp::NodeOptions::context set.  They will be automatically removed from the context when destructed.

Contexts may be shutdown by calling rclcpp::shutdown.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I tried to rephrase in 78181fd, feel free to review again!

@traversaro traversaro force-pushed the fix2106 branch 3 times, most recently from 78181fd to 084b423 Compare February 16, 2023 16:27
Copy link
Copy Markdown
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm!

Copy link
Copy Markdown
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

One really minor thing (one sentence per line), and this looks good to me.

@clalancette
Copy link
Copy Markdown
Contributor

This just needs a proper Signed-off-by line on the last commit, then we'll be good to merge it (assuming the Rpr job comes back green).

Signed-off-by: Silvio Traversaro <silvio@traversaro.it>
@traversaro
Copy link
Copy Markdown
Contributor Author

This just needs a proper Signed-off-by line on the last commit, then we'll be good to merge it (assuming the Rpr job comes back green).

Done, sorry but I got distracted between the "Apply comment of the reviewers" and the rebase for the signoff.

@clalancette
Copy link
Copy Markdown
Contributor

Since this is just a documentation update, I'm going to merge with just the Rpr job. Thanks for the improvement!

@clalancette clalancette merged commit 28e4b1b into ros2:rolling Feb 16, 2023
@traversaro traversaro deleted the fix2106 branch February 16, 2023 22:18
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.

Documentation issue: Context documentation refer to non-existing rclcpp::init_local function

3 participants