Thanks for this. Sorry for the delay. Some nits before it's mergeable:
On Sun, Oct 7, 2018 at 10:34 PM Lars Flitter
<password-st...@larsflitter.de> wrote:
> install-common:
> @install -v -d "$(DESTDIR)$(MANDIR)/man1" && install -m 0644 -v
> man/pass.1 "$(DESTDIR)$(MANDIR)/man1/pass.1"
> - @[ "$(WITH_BASHCOMP)" = "yes" ] || exit 0; install -v -d
> "$(DESTDIR)$(BASHCOMPDIR)" && install -m 0644 -v
> src/completion/pass.bash-completion "$(DESTDIR)$(BASHCOMPDIR)/pass"
> + @[ "$(WITH_BASHCOMP)" = "yes" ] || exit 0; \
> + install -v -d "$(DESTDIR)$(BASHCOMPDIR)" && \
> + trap 'rm -f src/completion/.bash' EXIT; \
> + sed
> 's:^SYSTEM_EXTENSION_DIR=.*:SYSTEM_EXTENSION_DIR="$(LIBDIR)/password-store/extensions":'
> src/completion/pass.bash-completion > src/completion/.bash && \
> + install -m 0644 -v src/completion/.bash
> "$(DESTDIR)$(BASHCOMPDIR)/pass"
Either clean up the other usage of this pattern to be multi-line in
preceding commit, or make this single line like the other.
> +_pass_extension_commands() {
> + if [[ -n $SYSTEM_EXTENSION_DIR ]]; then
> + find $SYSTEM_EXTENSION_DIR -name \*.bash -printf "%f " | sed
> 's/.bash//g'
> + fi
> +
> + if [[ $PASSWORD_STORE_ENABLE_EXTENSIONS == true ]]; then
> + find $EXTENSIONS -name \*.bash -printf "%f " | sed
> 's/.bash//g'
> + fi
> +}
These should use pure bash instead of find+sed. If you're concerned
about nested directories, you can enable extglob for the duration of
the function and the ** operator. Suffixes can be removed using the
normal bash % operator on the results. Also -d should be used instead
of -n, above.
> +
> +_pass_extension_completion_file() {
> + local extension_command=$1
> + if [[ $PASSWORD_STORE_ENABLE_EXTENSIONS == true ]] && [ -f
> $EXTENSIONS/$extension_command.bash.completion ]; then
Please always use [[ and not [. You can combine these two clauses into
a single one.
Also - elsewhere we use bash-completion, not bash.completion.
> + echo $EXTENSIONS/$extension_command.bash.completion
Missing quotation marks.
> + return 0
> + fi
> +
> + if [[ -n $SYSTEM_EXTENSION_DIR ]] && [ -f
> $SYSTEM_EXTENSION_DIR/$extension_command.bash.completion ]; then
> + echo $SYSTEM_EXTENSION_DIR/$extension_command.bash.completion
Same feedback as above. However, you probably should just be copying
the logic in pass' cmd_extension instead, where you just build the
full path, test which one exists, and source things opportunistically.
Copy and paste it, even.
> - local commands="init ls find grep show insert generate edit rm mv cp
> git help version ${PASSWORD_STORE_EXTENSION_COMMANDS[*]}"
> + local commands="init ls find grep show insert generate edit rm mv cp
> git help version $(_pass_extension_commands)"
Not sure this is such a good idea. I get that sometimes you want to
list the commands, it seems there are actually three cases:
- User hits tab in the state when listing all commands --> call
_pass_extension_commands to see what should be appended
- User hits tab in the state of a built-in command --> nothing.
- User hits tab in the state of an unknown command --> see if there
exists a bash completion for the extension that should be sourced for
that command.
Right now you're doing the first all the time, which I'm not sure is
so smart performance-wise.
> + # To add completion for an extension command create
> <COMMAND>.bash.completion in the extension folder that contains the
> completion code like this:
Same nit about filename template.
> + #
> + # COMPREPLY+=($(compgen -W "-o --option" -- ${cur}))
> + # _pass_complete_entries 1
> #
> - # and add the command to the
> $PASSWORD_STORE_EXTENSION_COMMANDS array
> - if [[ " ${PASSWORD_STORE_EXTENSION_COMMANDS[*]} " == *"
> ${COMP_WORDS[1]} "* ]] && type
> "__password_store_extension_complete_${COMP_WORDS[1]}" &> /dev/null; then
> - "__password_store_extension_complete_${COMP_WORDS[1]}"
> + if [[ " $(_pass_extension_commands)" == *" ${COMP_WORDS[1]}
> "* ]] && [ -n "$(_pass_extension_completion_file ${COMP_WORDS[1]})" ] ; then
Same nit as above.
> + source $(_pass_extension_completion_file
> ${COMP_WORDS[1]})
> fi
> else
> COMPREPLY+=($(compgen -W "${commands}" -- ${cur}))
> --
> 2.19.1
>
> _______________________________________________
> Password-Store mailing list
> Password-Store@lists.zx2c4.com
> https://lists.zx2c4.com/mailman/listinfo/password-store
_______________________________________________
Password-Store mailing list
Password-Store@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/password-store
