Add support for completing short flags (-x)

[chriswalz]

I have a setup like so:

Flags: map[string]complete.Predictor{
  "o": predict.Nothing,
  "upload-pack": predict.Nothing,
  ...
}

Now when I type a command like <cmd> -- [TAB] I see the below

I would expect to only see the long flags

[posener]

Hey Chris, Can you elaborate more on the ecosystem that you are using? How the completion is invoked? What OS? What shell? How is the complete command installed in the shell?

…

On Mon, Oct 5, 2020 at 7:20 AM Chris ***@***.***> wrote: I have a setup like so: Flags: map[string]complete.Predictor{ "o": predict.Nothing, "upload-pack": predict.Nothing, ... } Now when I type a command like <cmd> -- [TAB] I see the below [image: Screen Shot 2020-10-05 at 12 15 48 AM] <https://user-images.githubusercontent.com/6971318/95039905-0f876e80-06a0-11eb-9492-1753146d4243.png> I would expect to only see the long flags — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#127>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHAN7WAAMCUG6ZA3NDOBYTSJFCRDANCNFSM4SEFVFVA> .

[chriswalz]

Ecosystem: V2
OS: Mac OS Catalina
Shell: zsh
Installation: A Go binary is installed on users machine via this command curl -sf https://gobinaries.com/chriswalz/bit/bitcomplete | sh && COMP_INSTALL=1 bitcomplete

Here's the completion file if you're curious:
https://github.com/chriswalz/bit/blob/master/bitcomplete/complete.go

[posener]

I see,
In Go's standard flags you can use either a single dash, or double dash, and it acts the same.
This is why currently the complete library does not distinguish between a single dash and double dash.
Here is the flag suggestion code: https://github.com/posener/complete/blob/master/complete.go#L210
The comparison is only done over the flag name, and not over the dashes.
The fact that the user entered dashes indicates that it wants a flag, but then the library does not distinguishes between a single and double dashes - they are both valid options.

[chriswalz]

So if I understand correctly, there isn't a good workaround for this unless Go changes the way it handles flags.

I'm probably reading the code wrong but couldn't complete.Command take in Flags with the dashes included and then remove the dashes when appending them to the options slice here

[posener]

So if I understand correctly, there isn't a good workaround for this unless Go changes the way it handles flags.

Not exactly, this library is not directly related to Go's flags, but when I wrote it, I kept Go's flags idioms in mind. It makes sense to allow single-dash flags here, as an option.

One issue with adding this feature, is that the Completer interface has a ListFlags method that returns all flag names, and does not distinguish between single and double dash flags. And changes to this interface will break the library's API (e.g. return a list of some flag type instead of a list of strings, or add another method that returns the list of single dash flags...).

I'm probably reading the code wrong but couldn't complete.Command take in Flags with the dashes included and then remove the dashes when appending them to the options slice here

In the linked code, the completion remembers how many flags there were, and compare the flag names, and then output the number of dashes that the user entered plus the flag name. Since both single and double dashes are allowed, it tries to complete all available flags.

[posener]

Actually, there is another option, we can create an optional interface, in which we can add a function, for example ListShortFlags() []rune, which are flags of a single dash and a single character.

[chriswalz]

Cool depending on how much time I have, I could probably help out 👍

[chriswalz]

Hey @posener I wrote up a proof of concept here chriswalz@b376a83

Some notes:

1. it's a small code change but it would break backwards compatibility so I suppose there is more to be done. Could you integrate this into the library?
2. These two tests fail Test(t, cmp, "--foo ", []string{"false"}) Test(t, cmp, "--foo=", []string{"false"}) I didn't quite understand how they should work
3. I removed the help flag since the user should decide whether it should be added IMO

[posener]

Hey,
Thanks for trying to solve this.

1. Backward compatibility is important.
2. Can you please point out the the broken tests?
3. This is due to Go's flag library behavior, where the -h and --help flags not necessarily need to be explicitly defined, but could also be used when an ErrHelp is returned: https://github.com/golang/go/blob/afe7c8d0b25f26f0abd749ca52c7e1e7dfdee8cb/src/flag/flag.go#L945. Maybe the completion should not be there, I'm not sure about the right behavior.

[chriswalz]

1. Agreed, I'll try to figure out a way to not break backwards compatibility
2. The two tests I listed in point 2 are the tests.

[chriswalz]

@posener hey please check out this PR #129.

The idea is that instead of this:
gogo.Complete("go")

You use this to enable the old way of unix style (without breaking backwards compatibility):
os.Setenv("COMP_TRADITIONAL_UNIX_STYLE", "1")
gogo.Complete("go")

Currently these two tests are failing and I'm not quite sure why

	TestWithTraditionalUnixStyle(t, cmp, "--foo ", []string{"false"})
	TestWithTraditionalUnixStyle(t, cmp, "--foo=", []string{"false"})

Seems to be something related to p.Predict(prefix)

[posener]

Hi, sorry for keeping you waiting with the PR.

There is more into it, i think. I'm not sure that the current behavior is wrong - From what I see here, a single dash can be followed by multiple single dash flags, so we might want to support this as well? However, this behavior might confuse users, for example, what happens when there is a short flag that should also get a value? Or, do we want to support short flags, but not support getting multiple short flags?

The most elegant way I can see this being somehow solved is by extending the Completer interface with an optional interface:

// ShortFlagger is an optional interface for
type ShortFlager interface {
	ListShortFlags() []rune
}

Then, add to the Command struct another field: ShortFlags map[rune]Predictor, and another method: func (c *Command) ShortFlagList() []rune, such that it will implement the optional interface. Then, in the suggestFlag function, check for if the completer also implements the ShortFlagger interface.

But again, I'm not sure that having this option will not confuse users more than it will benefit. I really don't see the harm in the current implementation. How is this a deal breaker for you? Why is it so bad that the current completion will allow both -o, --o, -output and --output?

Thanks!

[chriswalz]

I'm not that familiar with optional interfaces but I believe I understand what you mean.

For context I'm partially wrapping git in https://github.com/chriswalz/bit. Ideally if I could tap into git's tab completions that would be even better and may be possible the more I think about it. Git uses the "traditional style" so that kinda forces me to use that style