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

Script auto-completion only ever completes options and never default bash completions #461

Closed
dwalluck opened this issue Sep 2, 2018 · 26 comments
Milestone

Comments

@dwalluck
Copy link
Contributor

dwalluck commented Sep 2, 2018

I have generated the bash completion script for my command. I have a command that looks like:

command [OPTIONS] FILE...

If I type $ command <TAB> I get $ command - and then hitting TAB twice to get $ command -<TAB><TAB> lists all of the short and long options. The proposed completions for the command options themselves look correct.

However, the completion only ever tries to complete the command options (as they begin with a -), and therefore, it never does not get a match and thus never falls back to the default completions.

It should be listing the default case which is files/directories and not supply a -. It should only try to complete the command options after I manually type a - (or --) on the command line. This is how (all) other command completions in bash seem to work.

Also, it would be nice to have --<SPACE> force it stop completing command options and fall back to the default competitions as this matches the behavior of the actual command.

@remkop
Copy link
Owner

remkop commented Sep 2, 2018

What you’re saying makes sense.
Do you think you’ll be able to provide a pull request for this?

@dwalluck
Copy link
Contributor Author

dwalluck commented Sep 2, 2018

I think this will do it, but I couldn't find where it actually tests for this issue (although it did not break any existing tests).

@remkop
Copy link
Owner

remkop commented Sep 2, 2018

PS. If you have ideas for more or better tests, they’ll be very welcome!

@remkop remkop added this to the 3.6 milestone Sep 2, 2018
@dwalluck
Copy link
Contributor Author

dwalluck commented Sep 2, 2018

It would require having bash on the testing machine and being able to interact with it to verify the completion, but I am not sure how feasible that is.

@dwalluck
Copy link
Contributor Author

dwalluck commented Sep 2, 2018

It is also be possible to complete positional parameters in the same way that options are being completed now in AutoComplete::generateOptionsCases.

I haven't looked into this yet, so I am not sure how hard it will be to implement, but I may at least file another issue. For now, the default completion should complete files/directories for all non-options, which is at least an improvement.

@dwalluck
Copy link
Contributor Author

dwalluck commented Sep 2, 2018

I think that I also need to test this with a command that uses COMMANDS since this won't begin with -.

@dwalluck
Copy link
Contributor Author

dwalluck commented Sep 2, 2018

The case where no options are given will have to be fixed. This small fix was apparently too good to be true.

If CURR_WORD does not start with -, then we must either have a subcommand or a positional parameter. I do not think we already have an index into which position we are at as COMP_CWORD is not enough since this includes the options.

@dwalluck
Copy link
Contributor Author

dwalluck commented Sep 2, 2018

Currently, #462 is repeating the bug that both options and commands are given as a possible completion at the same time. And because it is checking for -, it will never propose the subcommand at the right time.

We want to split options and subcommands into separate cases, with something like the following, except that this allows the subcommand to appear multiple times

  if [[ "${CURR_WORD}" == -* ]]; then
    COMPREPLY=( $(compgen -W "${FLAG_OPTS} ${ARG_OPTS}" -- ${CURR_WORD}) )
  else 
    COMPREPLY=( $(compgen -W "${COMMANDS}" -- ${CURR_WORD}) )
  fi

@remkop
Copy link
Owner

remkop commented Sep 3, 2018

Maybe we should clarify first what exactly the expected behaviour is.

Example program:

@Command(name = "better", subcommands = { Sub.class, Sub2.class, Sub3.class } )
class Main {
    @Option(names = { "-h", "--help"}) boolean help;
    @Option(names = { "-V", "--version"}) boolean version;
    @Option(names = { "-n", "--number"}) int number;
    @Parameters File[] files; // we want file completions
}

@Command(name = "sub") 
class Sub {
    @Option(names = { "-h", "--help"}) boolean help;
    @Option(names = { "-d", "--dir"}) File dir;
    @Parameters InetAddress[] hosts; // we want host completions
}

@Command(name = "sub2") 
class Sub2 {
    @Option(names = { "-n", "--num"}) int[] numbers;
    @Parameters(index = "0") InetAddress host; // we want host completions
    @Parameters(index = "1") File file; // and we want file completions
}

@Command(name = "sub3") 
class Sub3 {
    @Parameters int[] moreNumbers; // we want default completions
}
  1. What behaviour do we want to see if the user enters better <TAB>?
    I would say either only show all subcommands or show subcommands and file completions. What do you think?

  2. What about better sub <TAB>?
    I would say only show registered host names (from $( compgen -A hostname -- "" ) ) because there is only one positional parameter of the sub subcommand, and it is of type InetAddress.

  3. What about better sub2 <TAB>?
    Probably a combination of file and host completions? I don't think we need to try to find the ideal completion based on index of the positional parameter because positional parameters may be mixed with options on the command line; we would end up duplicating picocli's parsing logic in bash...

  4. What about better sub3 <TAB>?
    Probably the default completions.

Thoughts?

@dwalluck
Copy link
Contributor Author

dwalluck commented Sep 3, 2018

Here are my responses:

  1. In the first (or general) case, we immediately hit some problems, so let's save this for a later comment.
  2. I agree here, but remind me how better sub -<TAB> works. Does picocli accept only -hd here and not, say, -V? How about better sub -d -<TAB>? Do we propose -hd or -hVn or -hdVn? I am forgetting how picocli handles parsing of the global (main) options when we also have suboptions (subcommand options). This answer may be easy if picocli disallows global options from appearing after a subcommand.
  3. In this case, it is technically dependent on the position, where the first position must be a hostname and the second must be a filedir. The fact being that proposing files first would be wrong, I would rather have it propose only default completions unless or until it could be handled properly, rather than propose an incorrect option. And recall that a parameter has not only position, but arity. The actual software bash-completition provides a file with functions in the file /usr/share/bash-completion/bash_completion to, e.g., get the position of a word, but the current implementation does not rely on this external software, I believe. To handle complex cases, we'd probably want to switch to relying on it. But, I'd rather handle the easy or general cases first and above all.
  4. I agree here. We have no existing proposals, so we'd return an empty completion reply which will give the default completions.

@dwalluck
Copy link
Contributor Author

dwalluck commented Sep 3, 2018

Another issue with the existing code is that it has an outer and inner loop. I don't even think that this is accomplishing anything. Am I wrong? Could we not replace

  case ${CURR_WORD} in
    *)
      case ${PREV_WORD} in

with

case ${PREV_WORD} in

@dwalluck
Copy link
Contributor Author

dwalluck commented Sep 3, 2018

Comments for case 1 or the general case and what I believe is the first issue that ought to be fixed:

Assume first that better has no subcommands, then better <TAB> should at least produce default completions, and ideally, file competitions. Therefore, better <TAB> should not propose -hVn until one types better -<TAB>. Now assume that better has subcommands. As you stated, we immediately hit an ambiguity. In perhaps the most known case of a program with subcommands, git, it has the form git [options] <command> [<args>]. In other words, <command> is required, so that we do not have this ambiguity. What is considered a required subcommand? Is it simply that the main command have no @Parameters?

The easy case: if the subcommand is required, then propose only subcommands. The ambigous case: I would say that it should propose both subcommands and file completions, since either is allowed here, proposing the subcommands first for clarity.

Also, you have not mentioned this, but I believe that the command may only occur as the first word, Anything after the first word is considered a parameter. One problem with the existing picocli code and the naive fix is that it does not take position into account and so it is proposing the commands multiple times/in any position.

@dwalluck
Copy link
Contributor Author

dwalluck commented Sep 3, 2018

Another issue with the existing code is that it does not handle option-parameter separators (=). Actually, picocli allows changing of various options such as this which ideally needs to be reflected in the completion script or it may potentially break the completion. Currently, if you type = after a long option, the completion stops, so it must be a space. Traditionally, what bash-completion seems to do is to only propose completions with the separator attached, and it has some built-in support for this using _init_compleition -s or more generally _init_completion -n <separator>.

@dwalluck
Copy link
Contributor Author

dwalluck commented Sep 3, 2018

What about better sub2 ?
Probably a combination of file and host completions? I don't think we need to try to find the ideal completion based on index of the positional parameter because positional parameters may be mixed with options on the command line; we would end up duplicating picocli's parsing logic in bash...

If you use bash-completion, you can do something like:

local args
_count_args
case $args in
    1)
        _known_hosts_real "$cur"
       ;;
    2)
        _filedir
        ;;

since $args counts only the positional args whereas $cword is the position of any word on the command line.

@remkop
Copy link
Owner

remkop commented Sep 4, 2018

external dependency

I am reluctant to depend on bash-completion.

Maybe one idea is to let application authors decide whether they want to generate a completion script that works reasonably well and does not require any dependencies, OR whether they want to generate a more powerful completion script that requires their end users to install bash-completion.
This means picocli would need to support two script generators.

inner loop

This is to handle completion on option arguments. ${PREV_WORD} is the option, ${CURR_WORD} is potentially the option argument.
The completion first checks if CURR_WORD is an option, in which case we complete the option.
If not (and CURR_WORD matches *), we check all options with arguments again to see if we can complete the argument.
So I think it's needed.

option-parameter separator

You are correct, this is currently not handled well.
That is a potential improvement. (May deserve a separate ticket.)
This may require bash-completion (or maybe we can borrow code from bash-completion for this).
Perhaps it's as simple as adding complete nospace and recognizing case ${PREV_WORD} in -h|--host|-h=|--host= in the inner loop.

position-specific completion

(Like in the better sub2 example, where the first positional parameter is an InetAddress and the second positional parameter is a File.)
This is also a potential improvement. (And may also deserve a separate ticket.)
This may require bash-completion (or maybe we can borrow code from bash-completion for this).

subcommands

Picocli does not have the concept of required subcommands.

Is it possible to combine a built-in (file completion) with a predefined list of candidates (subcommand names)?

If that is possible, I like the idea of showing subcommands first, but I believe bash always sorts the completion candidates alphabetically.

If we cannot combine a built-in with a pre-defined list I think we should just show subcommands.

Also, you have not mentioned this, but I believe that the command may only occur as the first word, Anything after the first word is considered a parameter. One problem with the existing picocli code and the naive fix is that it does not take position into account and so it is proposing the commands multiple times/in any position.

I thought the current completion script correctly shows subcommands of the preceding command. For example, if Sub2 in the above example had a subcommand Sub2Sub, then better sub2 <TAB> would only suggest the sub2sub subcommand as a completion candidate, it should not suggest sub, sub2 or sub3. Is this not working?

global options

Picocli does not have the concept of global options. Options following a subcommand need to be defined in that subcommand.

@dwalluck
Copy link
Contributor Author

dwalluck commented Sep 5, 2018

Maybe one idea is to let application authors decide whether they want to generate a completion script that works reasonably well and does not require any dependencies, OR whether they want to generate a more powerful completion script that requires their end users to install bash-completion.
This means picocli would need to support two script generators.

The bash-completion software itself is under GPLv2+ (as opposed to ASL 2.0 for picocli). I just don't want to be in the situation such that for every new feature we want to add, we are just re-implementing what is already in bash-completion.

I still wasn't sure if #461 (comment) was the correct fix. I will have to test it again. I thought that it was proposing sub sub2 sub3 more than once (which is probably expected if we are not verifying that the given arg is the first arg). This is the problem I was trying to describe later on in the comment about subcommands.

inner loop
This is to handle completion on option arguments. ${PREV_WORD} is the option, ${CURR_WORD} is potentially the option argument.

It does not appear to be needed. You can remove the outer loop and not lose functionality. It seems to want to handle the case where you have an empty ("") ${CUR_WORD}, but this case is already handled automatically.

This is also a potential improvement. (And may also deserve a separate ticket.)
This may require bash-completion (or maybe we can borrow code from bash-completion for this).

Yes, let's push this to a separate issue. This ticket should just be to handle existing features correctly and not to add new features.

Is it possible to combine a built-in (file completion) with a predefined list of candidates (subcommand names)?

If that is possible, I like the idea of showing subcommands first, but I believe bash always sorts the completion candidates alphabetically.

Yes, you can combine them, but you are correct in that it will always sort them. When I said to propose them "first", it apparently doesn't matter if you propose them as "${FILES} ${SUBCOMMANDS}" or as "${SUBCOMMANDS} ${FILES}", or, at least when you hit <TAB> they are sorted alphabetically.

@remkop
Copy link
Owner

remkop commented Sep 5, 2018

Question about that last point, is it possible to combine a built-in completion (suggest all files in the current directory) with candidates from our script (${SUBCOMMANDS})?

@dwalluck
Copy link
Contributor Author

dwalluck commented Sep 5, 2018

Question about that last point, is it possible to combine a built-in completion (suggest all files in the current directory) with candidates from our script (${SUBCOMMANDS})?

You just add both files and commands to the COMPREPLY, but this means that it won't ever trigger -o default in that case.

For example:

COMPREPLY="${COMMANDS}"
compopt -o filenames
COMPREPLY+=...

The default replies will not be triggered if COMPREPLY is set, but this is usually files/dirs anyway. I do not know of a way to manually trigger the default replies.

@remkop
Copy link
Owner

remkop commented Sep 6, 2018

Ok, please let me know how your testing works out.

dwalluck added a commit to dwalluck/picocli that referenced this issue Sep 6, 2018
dwalluck added a commit to dwalluck/picocli that referenced this issue Sep 6, 2018
dwalluck added a commit to dwalluck/picocli that referenced this issue Sep 6, 2018
@dwalluck
Copy link
Contributor Author

dwalluck commented Sep 6, 2018

Ok, please let me know how your testing works out.

Sorry for the repeated force push commands. Is there a command in the picocli distribution itself that can be used to test the completion for subcommands? In any case, my own tests seem to work as expected if you want to verify now.

Do you want a separate pull request for removing the inner loop?

@dwalluck
Copy link
Contributor Author

dwalluck commented Sep 6, 2018

If you have an option that should require an argument, it will propose the subcommand unless it has some explicit types (such as an enum). But, I don't think that this is a regression.

dwalluck added a commit to dwalluck/picocli that referenced this issue Sep 6, 2018
remkop pushed a commit that referenced this issue Sep 7, 2018
@remkop
Copy link
Owner

remkop commented Sep 7, 2018

I merged the PR. Thanks very much! (I will add an entry to RELEASE-NOTES.md soon)

This discussion also revealed other points where the completion can be further improved:

Correct me if I'm wrong or missed something.
Do you feel like helping out with any of these? :-)

@dwalluck
Copy link
Contributor Author

dwalluck commented Sep 7, 2018

add support for completing positional parameters in addition to named options. This may be the most challenging as it requires the completion script to determine the position index in user input that contains a mixture of subcommands, positional parameters, named options and option parameters. (I raised #468 for this)

I think that there should also be an issue about fixing the main case statement.

For #468, having a mixture isn't so bad because each subcommand is processed separately and the index should be local to each subcommand. Then, we need to offset the main index by one based on whether we have a command or a parameter for the first argument.

However, in #461 (comment), I believe that you mentioned a case where the first parameter can be either parameter or a subcommand, so this part may be tricky. I am actually unsure how picocli resolves this case. For example, it currently allows a command of the form better <subcommand|files...> ? This is ambiguous, particularly if one of the subcommands matches one of the available file or directory names.

@remkop
Copy link
Owner

remkop commented Sep 7, 2018

There is ambiguity, and it’s resolved by matching subcommands first, then options (and parameters) and finally (if it couldn’t be matched to any of these), it must be a positional parameter.

@dwalluck
Copy link
Contributor Author

dwalluck commented Sep 9, 2018

If that is possible, I like the idea of showing subcommands first, but I believe bash always sorts the completion candidates alphabetically.

By the way, complete has a -o nosort option which may be relevant here.

@remkop
Copy link
Owner

remkop commented Sep 9, 2018

Nice find!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants