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

override parse_known_args for completion to work with partial argparse tree #446

Merged
merged 3 commits into from
Jan 31, 2020

Conversation

dirk-thomas
Copy link
Member

Fixes regression from #436.

…e tree

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
@dirk-thomas dirk-thomas added the bug Something isn't working label Jan 30, 2020
@dirk-thomas dirk-thomas self-assigned this Jan 30, 2020
Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

Now, verb autocompletion and completion of the following arguments work well.
But completion of the command stopped working (it do work on master).

e.g.:

ros2 no  # Did autocomplete well to `node` in master, not on this branch.
ros2 launch demo_ # Do autocomplete well to `demo_nodes_*` now, not on master. 
ros2 launch demo_nodes_cpp talker_  # Do autocomplete well now, not on master.

I'm not pretty sure of what's the problem (I would have to check argparse documentation).

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

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

It definetly works and code looks fine, but I can't tell if there's a better way of doing this or not.
Adding a second reviewer sounds like a good idea, maybe @mjcarroll who reviewed #436.

ros2cli/ros2cli/command/__init__.py Outdated Show resolved Hide resolved
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
@dirk-thomas
Copy link
Member Author

I can't tell if there's a better way of doing this or not.

colcon is doing pretty much the same: https://github.com/colcon/colcon-argcomplete/blob/86206dc187b1c5a318439e7010789a8979e5d0f7/colcon_argcomplete/argument_parser/argcomplete/__init__.py#L72-L80

Therefore I will get this merged. If anyone has a better proposal we can always follow up with a separate PR.

@dirk-thomas dirk-thomas merged commit bd2aa69 into master Jan 31, 2020
@delete-merged-branch delete-merged-branch bot deleted the dirk-thomas/fix-completion branch January 31, 2020 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants