Skip to content

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

Merged
dirk-thomas merged 3 commits intomasterfrom
dirk-thomas/fix-completion
Jan 31, 2020
Merged

override parse_known_args for completion to work with partial argparse tree#446
dirk-thomas merged 3 commits intomasterfrom
dirk-thomas/fix-completion

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 requested a review from ivanpauno January 30, 2020 17:36
@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>
@dirk-thomas dirk-thomas requested a review from ivanpauno January 30, 2020 21:24
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.

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