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

allow completion for several packages after --whitelist or --blacklist #209

Closed
wants to merge 4 commits into from

Conversation

rhaschke
Copy link
Contributor

@rhaschke rhaschke commented Jun 3, 2015

This is an extension to #206 allowing bash completion not only for the first but for any number of packages after --whitelist or --blacklist.

@jbohren
Copy link
Contributor

jbohren commented Jun 15, 2015

@rhaschke Is it possible to do this without having to search backwards through the arglist?

@rhaschke
Copy link
Contributor Author

If you have any idea to do it without backwards search, I'm curious to learn about it.
IMHO we need to search backwards for the latest option string. Otherwise we cannot know the option context. Note, that the search is only performed in case of catkin config.
Using "forward search", e.g. checking the third element of the argument list to be --whitelist or --blacklist, will not scale to an arbitrary number of additional options.

@jbohren
Copy link
Contributor

jbohren commented Jun 16, 2015

Yeah, I know what you mean. Well, I like this enhancement, but haven't tried it out yet. In the future, it'd be cool to use that _last_option function for more places in the autocompletion code.

@rhaschke
Copy link
Contributor Author

Yes, that's why I decided for a generic _last_option() function.

@rhaschke
Copy link
Contributor Author

I rewrote the auto-completion script to parse options from catkin's help output itself.
This way, completion is always up-to-date and complete. The code follows the one in /etc/bash_completion that defines a generic _longopt function to do help-based completion.
Also, the _init_completion function comes from there, allowing standard completion for shell variables and redirection.

@jbohren
Copy link
Contributor

jbohren commented Jun 30, 2015

I rewrote the auto-completion script to parse options from catkin's help output itself.

Cool! How's the speed when doing that? Would it be worth pre-compiling any of the completion options?

@rhaschke
Copy link
Contributor Author

I didn't notice any performance issues. Try yourself ;-)
Thinking about caching or precompiling options, I'm actually not sure how to achieve that. You would need to store them locally on the file system, because bash completion is triggered on the fly without any memory about the past, isn't it?

@wjwwood
Copy link
Member

wjwwood commented Jun 30, 2015

Would it be better to have a mode where the catkin command dumps a more readable version of the args, for example I think this is how this works:

https://argcomplete.readthedocs.org/en/latest/

@rhaschke
Copy link
Contributor Author

rhaschke commented Jul 1, 2015

Interesting link. It seems to provide pretty much everything needed.
However, this requires (again) a rewrite of the code (and the additional python package to be installed). As I am happy with the proposed patch, I will not put more effort into this for now.

@jbohren
Copy link
Contributor

jbohren commented Jul 1, 2015

@rhaschke Thanks for these contributions!

I'll test it out and @wjwwood and I will pick out the parts that meet all of our needs.

@wjwwood
Copy link
Member

wjwwood commented Dec 21, 2015

Several of these commits have been picked to master, I'm gonna close this for now. Please open a new pr if there are still changes that need to be made.

@wjwwood wjwwood closed this Dec 21, 2015
@NikolausDemmel
Copy link
Member

@wjwwood: Maybe I'm missing something, but I only see the original #206 in master, not the extension to complete multiple packages.

@wjwwood
Copy link
Member

wjwwood commented Dec 21, 2015

@NikolausDemmel oh you might be right.

@wjwwood wjwwood reopened this Dec 21, 2015
@jbohren
Copy link
Contributor

jbohren commented Feb 16, 2016

@rhaschke The current state of this PR doesn't work on zsh due to the use of _init_completion

@rhaschke
Copy link
Contributor Author

If there is a conflict of catkin_tools-completion.bash with zsh, then we should have a separate completion script for zsh, shouldn't we? As I don't use zsh, I cannot contribute here.

@jbohren
Copy link
Contributor

jbohren commented Feb 16, 2016

Right now the completion works in both shells, zsh does have some more advanced completion options, but right now it's easier to maintain a single completion script.

@jbohren
Copy link
Contributor

jbohren commented Apr 29, 2016

@rhaschke I added these commits to #365, which also adds a zsh-specific completion script.

@jbohren jbohren closed this Apr 29, 2016
@rhaschke rhaschke mentioned this pull request May 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants