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.

Sign up for GitHub

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

Add verbose info for topic list #351

Merged
merged 10 commits into from
Apr 1, 2021
+58 −6
Merged

Add verbose info for topic list #351

Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7eedd6e
Add help argument for verbose in topic list.
evshary Oct 6, 2019
3014292
Able to show verbose info of topic list.
evshary Oct 6, 2019
0b14c84
Refactor the code and use function instead.
evshary Oct 9, 2019
d32b728
Use underscore to replace unused variables
evshary Oct 14, 2019
5682647
Use the same indentation for consistency
evshary Oct 14, 2019
b61a605
Use NodeStrategy instead of DirectNode for showing topic list.
evshary Oct 20, 2019
a4c438b
Remove blank line.
evshary Mar 19, 2021
8f0e5a3
Add test for topic list verbose.
evshary Mar 20, 2021
8d176cb
Restruct the code.
evshary Mar 23, 2021
5713a51
Restruct the code.
evshary Mar 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 31 additions & 6 deletions ros2topic/ros2topic/verb/list.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,18 @@
from ros2topic.verb import VerbExtension


def show_topic_info(topic_info, isPub):
Copy link
Member

Choose a reason for hiding this comment

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

A few nitpicks

  • isPub isn't really Pythonic. Could you do is_publisher instead? Although, maybe there's a better way to structure this as "subscription" isn't really the opposite of "publisher", as anything else would not be a publisher.
  • I prefer longer names, as I find it much more readable. For example, rather than using cnt, you can just write count.
  • I think this function should go to the bottom of the file, rather than be at the top. You could also make it a static method of the ListVerb class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you.

evshary marked this conversation as resolved.
Show resolved Hide resolved
print('\n' + ('Published' if isPub else 'Subscribed') + ' topics:')
for (topic_name, topic_types, pub_cnt, sub_cnt) in topic_info:
cnt = pub_cnt if isPub else sub_cnt
if cnt:
topic_types_formatted = ', '.join(topic_types)
cnt_str = str(cnt) + ' ' + ('publisher' if isPub else 'subscriber') \
+ ('s' if cnt > 1 else '')
msg = ' * {topic_name} [{topic_types_formatted}] {cnt_str}'
print(msg.format_map(locals()))
evshary marked this conversation as resolved.
Show resolved Hide resolved


class ListVerb(VerbExtension):
"""Output a list of available topics."""

Expand All @@ -34,19 +46,32 @@ def add_arguments(self, parser, cli_name):
parser.add_argument(
'--include-hidden-topics', action='store_true',
help='Consider hidden topics as well')
parser.add_argument(
'-v', '--verbose', action='store_true',
help='list full details about each topic')
evshary marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps the command should change to something like --show-connections (maybe there's a better name) or be broken into --show-subscriptions and --show-publishers. You could also have a --verbose command that enables everything, but it would be nice to have the ability to enable things individually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer to add this feature in another PR.


def main(self, *, args):
topic_info = []
with NodeStrategy(args) as node:
topic_names_and_types = get_topic_names_and_types(
node=node,
include_hidden_topics=args.include_hidden_topics)
for (topic_name, topic_types) in topic_names_and_types:
pub_cnt = node.count_publishers(topic_name)
sub_cnt = node.count_subscribers(topic_name)
fujitatomoya marked this conversation as resolved.
Show resolved Hide resolved
topic_info.append((topic_name, topic_types, pub_cnt, sub_cnt))
Copy link
Member

Choose a reason for hiding this comment

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

To me, having a list of tuples with four elements isn't desirable. In topic_names_and_types, there are two and they're right there in the variable name. Maybe there is a better way to do this. Perhaps, just compute them as you need them in the show_topic_info function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why I compute them here is that I need the NodeStrategy. I'm not sure how to pass NodeStrategy to show_topic_info is a better way.

def show_topic_info(args, topic_info, isPub):
    with NodeStrategy(args) as node:
....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@audrow I add the if-condition to avoid running count_publishers and count_subscribers while there is no verbose. I'm not sure whether it's a better way, but I try to avoid pass args into another function just for with NodeStrategy(args) as node.
What do you think about it?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good.


if args.count_topics:
print(len(topic_names_and_types))
elif topic_names_and_types:
for (topic_name, topic_types) in topic_names_and_types:
msg = f'{topic_name}'
topic_types_formatted = ', '.join(topic_types)
if args.show_types:
msg += f' [{topic_types_formatted}]'
print(msg)
if args.verbose:
show_topic_info(topic_info, isPub=True)
show_topic_info(topic_info, isPub=False)
else:
for (topic_name, topic_types, _, _) in topic_info:
msg = '{topic_name}'
topic_types_formatted = ', '.join(topic_types)
if args.show_types:
msg += ' [{topic_types_formatted}]'
print(msg.format_map(locals()))