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

executor.add_node() returns a bool #216

Merged
merged 1 commit into from
Aug 8, 2018
Merged

executor.add_node() returns a bool #216

merged 1 commit into from
Aug 8, 2018

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Aug 8, 2018

The documentation for executor.add_node() says it returns True if the node was added; however it currently just returns None. This fixes the function, and also makes it so the guard condition to rebuild the waitset is only triggered if the node really was added.

@sloretz sloretz added bug Something isn't working in review Waiting for review (Kanban column) labels Aug 8, 2018
@sloretz sloretz self-assigned this Aug 8, 2018
@sloretz
Copy link
Contributor Author

sloretz commented Aug 8, 2018

CI

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

Copy link
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -230,6 +230,12 @@ def __await__(self):
rclpy.spin_once(self.node, timeout_sec=0)
self.assertTrue(did_return)

def test_executor_add_node(self):
self.assertIsNotNone(self.node.handle)
Copy link
Member

Choose a reason for hiding this comment

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

I see that this matches the style in this file but in general we should use a plain assert instead (the recommended way with pytest). Nothing needs to happen for this patch though...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Roger. I'll make sure to use a plain assert in the future.

@sloretz sloretz merged commit a70e787 into master Aug 8, 2018
@sloretz sloretz deleted the add_node_return_bool branch August 8, 2018 23:06
@sloretz sloretz removed the in review Waiting for review (Kanban column) label Aug 8, 2018
@sloretz sloretz mentioned this pull request Aug 15, 2018
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants