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 completion for parameter name arguments #364

Merged
merged 3 commits into from
Oct 17, 2019

Conversation

dirk-thomas
Copy link
Member

For the get and delete verbs. Also naming the argument parameter_name (rather than name) in the usage.

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
@dirk-thomas dirk-thomas added enhancement New feature or request in review Waiting for review (Kanban column) labels Oct 17, 2019
@dirk-thomas dirk-thomas self-assigned this Oct 17, 2019
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

My sole concern is response latency. How long does it take to provide suggestions? I ask because I've seen ros2 topic almost blocking while retrieving topics. DirectNode brings up a node on construction and tears it down on context leave -- maybe keeping it around may speed up subsequent queries, not the first though.

Also, why not adding completion for the set verb?

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
@dirk-thomas
Copy link
Member Author

dirk-thomas commented Oct 17, 2019

My sole concern is response latency. How long does it take to provide suggestions?

It takes as long as it takes to spin up the node and wait for the service.

I ask because I've seen ros2 topic almost blocking while retrieving topics.

In this case the delay happens when you use completion. If you don't it doesn't affect the call.

DirectNode brings up a node on construction and tears it down on context leave -- maybe keeping it around may speed up subsequent queries, not the first though.

It would be great if instead this information would be queried over the node daemon but that kind of API is not available atm.

why not adding completion for the set verb?

Added in 96bee44.

@hidmic
Copy link
Contributor

hidmic commented Oct 17, 2019

It would be great if instead this information would be queried over the node daemon but that kind of API is not available atm.

Yeah, that's what I was thinking of. Alright, food for thought.

Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

LGTM

@dirk-thomas
Copy link
Member Author

Sanity linter check:

  • Linux: Build Status

@dirk-thomas dirk-thomas merged commit 491a162 into master Oct 17, 2019
@delete-merged-branch delete-merged-branch bot deleted the dirk-thomas/parameter-name-completion branch October 17, 2019 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request in review Waiting for review (Kanban column)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants