-
Notifications
You must be signed in to change notification settings - Fork 225
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
Count publishers and subscribers #183
Conversation
Is this still a WIP? Let us know when this and related pr's are ready for review. |
Sorry for closing, misclick on my part. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Just a couple nitpick comments
rclpy/test/test_node.py
Outdated
self.node.count_subscribers('42') | ||
with self.assertRaisesRegex(ValueError, 'is invalid'): | ||
self.node.count_publishers('42') | ||
|
||
def test_node_logger(self): | ||
node_logger = self.node.get_logger() | ||
self.assertEqual(node_logger.name, 'my_ns.my_node') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mind updating this line to use TEST_NODE
and TEST_NAMESPACE
too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validate_topic_name(fq_topic_name) | ||
return func(self.handle, fq_topic_name) | ||
|
||
def count_publishers(self, topic_name): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mind adding a doc string to this and count_subscribers
? I realize most functions here don't have one, but adding them to new functions is an improvement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Nickolaim Would you mind rebasing your branch with master? CI looks ok except for test failures caused by not having #182. |
Sure, will do later today. I am relatively new to github; for the request I plan to add remote upstream that points to ros2/rclpy and merge the changes from it. Does it sound good? |
I assume here your remote is called
|
@Nickolaim adding on to @Karsten1987's comment, assuming your fork is called
then after rebasing
|
I rebased this PR with the master. |
Looks like all the builds have passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Nickolaim !
This is a partial implementation of ros2/ros2cli#60. I marked it as a WIP since I expect some comments. Plan to merge changes later and do another PR.
Connects to ros2/ros2cli#60