Completion on short option names allows for exploration of short and long option names #228
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- previously it would return only a list of short option names - now it returns both short and long option names; in the case when an option has both a short and long name, it will return only the long name Signed-off-by: Nick Wei <[email protected]>
n4wei
changed the title
jessevdk
requested changes
May 21, 2017
There was a problem hiding this comment.
completeShortNamesis no longer an accurate name, probably needs to becompleteNamesForShortPrefix(and rename the long one tocompleteNamesForLongPrefix)- I would like that there is still a single code path for completion matching (now code has been duplicated). E.g. re-using
completeOptionNames, but passing a flag whether or not to consider short names as well, or maybe just matching on the actual result name + prefix?
completion.go
Outdated
| repeats := map[string]bool{} | ||
|
|
||
| for name, opt := range s.lookup.longNames { | ||
| if !opt.Hidden { |
There was a problem hiding this comment.
This should still check for match prefix, just like completeOptionName does
completion.go
Outdated
| } | ||
|
|
||
| for name, opt := range s.lookup.shortNames { | ||
| if _, exist := repeats[name]; !exist && !opt.Hidden { |
- preferred more sensible names for completeShortNames and completeLongNames; changed to completeNamesForShortPrefix and completeNamesForLongPrefix - removed code duplication - match prefix is enforced
|
@jessevdk Thanks for providing the feedback. I have made the requested changes. Please review the changes. |
|
Not sure I completely understand the GitHub UI: Changes were requested, have been applied, the wait is now for a re-review by @jessevdk, of niet? |
jessevdk
requested changes
Jul 14, 2017
completion.go
Outdated
| for name, opt := range s.lookup.longNames { | ||
| if strings.HasPrefix(name, match) && !opt.Hidden { | ||
| results = append(results, Completion{ | ||
| Item: "--" + name, |
There was a problem hiding this comment.
This should use the correct platform dependent prefix (defaultLongOptDelimiter)
completion.go
Outdated
| for name, opt := range s.lookup.shortNames { | ||
| if _, exist := repeats[name]; !exist && strings.HasPrefix(name, match) && !opt.Hidden { | ||
| results = append(results, Completion{ | ||
| Item: "-" + name, |
There was a problem hiding this comment.
This should use the correct platform dependent short option prefix (defaultShortOptDelimiter)
Signed-off-by: Stu Pollock <[email protected]>
|
@jessevdk Tanks for the feedback. We have made the suggested changes. Please review at your earliest convenience. |
jessevdk
approved these changes
Jul 20, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR addresses feature request #224.
option has both a short and long name, it will return only the long name