Info verb for ros2topic #88
Conversation
ros2topic/test/test_info.py
Outdated
|
|
||
| from ros2topic.verb.info import InfoVerb | ||
|
|
||
| NODE_NAME = 'bar' |
There was a problem hiding this comment.
This variable doesn't seem to be used anywhere?
There was a problem hiding this comment.
Yes, it is unused, deleted. Interesting that flake8 did not detect it.
Now why this code existed at some point and addressing the comment on the value of the test. So this test is not super valuable, but still useful. It will fail if node.count_subscribers() fails. That's useful. Also having a test that documents expected output is also useful. It is a reminder that changing output format will break users who do text parsing.
I tried to come with more complicated test (NODE_NAME is a leftover), but creating publisher, subscriber and calling info verb will require 2 additional threads + syncronization, and it was just too much for such trivial thing(or I just don't know how to do it simpier). Better testing is available in PR in rclpy.
ros2topic/ros2topic/verb/info.py
Outdated
| count_subscribers = node.count_subscribers(topic_name) | ||
| print('Topic: %s' % topic_name) | ||
| print('Publishers count: %d' % count_publishers) | ||
| print('Subscribers count: %d' % count_subscribers) |
There was a problem hiding this comment.
Should be "Publisher count" / "Subscriber count" (singular instead of plural).
ros2topic/test/test_info.py
Outdated
| from ros2topic.verb.info import InfoVerb | ||
|
|
||
| NODE_NAME = 'bar' | ||
| TOPIC_NAME = '/foo' |
There was a problem hiding this comment.
Please use a topic name which is specific to this test, e.g. "test_ros2_topic_cli".
dirk-thomas
added
in progress
- Update the output text - Rename the test topic name - Delete obsolete code
|
Anything else I should do for this PR and for ros2/rclpy#183? |
ros2topic/test/test_info.py
Outdated
| string_io_stdout = StringIO() | ||
| sys.stdout = string_io_stdout | ||
| yield string_io_stdout | ||
| sys.stdout = real_stdout |
There was a problem hiding this comment.
Could this be replaced with contextlib.redirect_stdout?
There was a problem hiding this comment.
Fixed. Log is here: https://gist.github.com/Nickolaim/6ca58a2454fbd0b4e6530d475106b349.
|
Comment addressed. |
dirk-thomas
added
in review
|
FYI: ros2/rclpy#183 has been merged. |
|
Thank you for adding this feature. |
Counts publishers and subscribers. The second part of #60.
Connects to #60