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

add type completer for 'topic pub' and 'service call' #64

Merged
merged 1 commit into from
Dec 3, 2017

Conversation

dirk-thomas
Copy link
Member

@dirk-thomas dirk-thomas commented Dec 3, 2017

Connect to #63. Fixes #63.

@dirk-thomas dirk-thomas added the in review Waiting for review (Kanban column) label Dec 3, 2017
@dirk-thomas dirk-thomas self-assigned this Dec 3, 2017
@mikaelarguedas
Copy link
Member

code change looks good overall. Though it would be great to hide the --repeat option if only a single service type matches.
e.g.

$ ros2 service call /add_two_ints <TAB><TAB> 
example_interfaces/AddTwoInts  -r  --repeat

I'd expect this to just complete the matching service name

@dirk-thomas
Copy link
Member Author

Though it would be great to hide the --repeat option if only a single service type matches.

I don't think that is technically possible (the results of one completer deciding to ignore the results of other completers). Anyway I wouldn't consider this within the scope of this PR.

@mikaelarguedas
Copy link
Member

fair enough. Then maybe for another PR we can consider providing the same argument to ros2 topic pub ? Because it's a bit surprising from the user side to have a different behavior

@dirk-thomas
Copy link
Member Author

Then maybe for another PR we can consider providing the same argument to ros2 topic pub ?

Sure, topic pub currently publishes with a fixed rate of once every second. A pull request to make this configurable would be great.

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.

code change looks good to me. I have one request on the docstrings before merging



class ServiceTypeCompleter:
"""Callable returning an existing service type or all service types."""
Copy link
Member

Choose a reason for hiding this comment

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

Nit: "an existing service type" I would recommend the documentation that it's returning the service type matching the passed service_name or all types (same for topics below)

Copy link
Member Author

@dirk-thomas dirk-thomas Dec 3, 2017

Choose a reason for hiding this comment

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

That sounds good but exceeds the one line rule for the docblock 😉

@dirk-thomas dirk-thomas merged commit baf1075 into master Dec 3, 2017
@dirk-thomas dirk-thomas deleted the add_type_completer branch December 3, 2017 20:14
@dirk-thomas dirk-thomas removed the in review Waiting for review (Kanban column) label Dec 3, 2017
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.

2 participants