Skip to content

Conversation

@hidmic
Copy link
Contributor

@hidmic hidmic commented Jun 2, 2020

Further improvements on top of #1043.

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

@hidmic hidmic requested review from Blast545 and brawner June 2, 2020 20:33
// Was added to default group
shared_node->remove_waitable(fake_shared_ptr, nullptr);
} else {
// Was added to a specfic group
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Was added to a specfic group
// Was added to a specific group

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in c19ba06.

Copy link
Contributor

@brawner brawner left a comment

Choose a reason for hiding this comment

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

Looks good to me, if you can fix the couple of typos.

Signed-off-by: Michel Hidalgo <[email protected]>
@hidmic
Copy link
Contributor Author

hidmic commented Jun 3, 2020

@jacobperron for approval.

Signed-off-by: Michel Hidalgo <[email protected]>
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

I only reviewed the changes to create_client.hpp and create_server.hpp (they LGTM).

@hidmic
Copy link
Contributor Author

hidmic commented Jun 8, 2020

Going in!

@hidmic hidmic merged commit 6e8aaa2 into master Jun 8, 2020
@delete-merged-branch delete-merged-branch bot deleted the hidmic/rclcpp_action-coverage branch June 8, 2020 14:03
DensoADAS pushed a commit to DensoADAS/rclcpp that referenced this pull request Jul 7, 2020
ahcorde pushed a commit that referenced this pull request Oct 22, 2020
ahcorde pushed a commit that referenced this pull request Oct 26, 2020
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.

5 participants