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

Add suggestions support #977

Merged
merged 2 commits into from May 17, 2022
Merged

Add suggestions support #977

merged 2 commits into from May 17, 2022

Conversation

saschagrunert
Copy link
Member

Motivation

The new option app.Suggest enables command and flag suggestions via the jaro-winkler distance algorithm. Flags are scoped to their appropriate commands whereas command suggestions are scoped to the current command level.

Closes #895

Release Notes

Add "Did you mean" suggestion support to flags and commands. This feature can be enabled by setting app.Suggest = true.

Changes

  • Added a new suggestions.go file where all the implementation details go in
  • Changed critical paths to show-up the suggestions
  • Added test cases as well

Testing

Added unit tests :)

Reviewer Guidelines

The scope of the suggestions should be correct, that we do not suggest flags from other sub-commands.

@codecov
Copy link

codecov bot commented Dec 5, 2019

Codecov Report

Merging #977 into master will increase coverage by 0.14%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #977      +/-   ##
==========================================
+ Coverage   72.91%   73.06%   +0.14%     
==========================================
  Files          33       34       +1     
  Lines        2481     2539      +58     
==========================================
+ Hits         1809     1855      +46     
- Misses        565      573       +8     
- Partials      107      111       +4     
Impacted Files Coverage Δ
suggestions.go 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b7e4e0...002bde2. Read the comment docs.

asahasrabuddhe
asahasrabuddhe previously approved these changes Dec 7, 2019
Copy link
Member

@asahasrabuddhe asahasrabuddhe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@coilysiren coilysiren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the code, and love that the suggestions stuff is mostly in it's own file 🙏

There's 2 more things I'd like to see from this PR:

  • for the total rest coverage in the PR to be increasing, rather than decreasing.
  • for suggestions to be added to the v2 manual. I'll stand up a PR for release v2.1 to support this.

@AudriusButkevicius
Copy link
Contributor

Should this be an out of the box OnUsageError handler ? Just wondering, not proposing.

@saschagrunert
Copy link
Member Author

Should this be an out of the box OnUsageError handler ? Just wondering, not proposing.

Seems a matter of taste to me, so I'm open for a vote :)

Code coverage seems broken for now, see #985 (comment)

@saschagrunert
Copy link
Member Author

@coilysiren
Copy link
Member

Minor request: can you avoid force pushing branches that I've already reviewed? I can code review faster when I can use Github's "commits since last push" feature, and force pushing breaks that.

@saschagrunert
Copy link
Member Author

Minor request: can you avoid force pushing branches that I've already reviewed? I can code review faster when I can use Github's "commits since last push" feature, and force pushing breaks that.

Sure, I can do this.

coilysiren
coilysiren previously approved these changes Dec 24, 2019
Copy link
Member

@coilysiren coilysiren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀 only a few non-blocking suggestions!

suggestions.go Show resolved Hide resolved
suggestions.go Show resolved Hide resolved
suggestions.go Show resolved Hide resolved
// command name
func suggestCommand(commands []*Command, provided string) (suggestion string) {
distance := 0.0
for _, command := range commands {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(nice to have) I'm not sure about this suggestion, but: would it be a good idea to consider suggesting sub commands too? I'm imagining an example CLI that has exactly these 3 commands

docker info
docker info containers
docker info images

and I input this

docker containers

I would be expecting to get a suggestion like

did you mean: docker info containers?

whereas I believe this code with give me

did you mean: docker info?

is that a correct assumption?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is correct that it would only suggest docker info in that case, because can only suggest one command with the current implementation. I'm also not sure if we should add such a traversal suggestion to the users. It might be helpful but it also might result in confusing them. 😁

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^^ fair!

@coilysiren
Copy link
Member

👀

@saschagrunert
Copy link
Member Author

I rebased on top of the latest master. Do you think we can move forward with this, @lynncyrin @AudriusButkevicius @asahasrabuddhe ?

@AudriusButkevicius
Copy link
Contributor

So my only comment is that "Suggest" is a poor name, we have UseXYZ everywhere, so the property could be more descriptive.

@saschagrunert
Copy link
Member Author

So my only comment is that "Suggest" is a poor name, we have UseXYZ everywhere, so the property could be more descriptive.

Hm, how about EnableFlagAndCommandSuggestions ? 🤔

@AudriusButkevicius
Copy link
Contributor

Sounds better. Perhaps others have suggestions.

@coilysiren
Copy link
Member

EnableSuggestions? I'm fine with any of the options mentioned, though!

coilysiren
coilysiren previously approved these changes Feb 9, 2020
Copy link
Member

@coilysiren coilysiren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:seal-of-approval:

pending the tests passing ^^

The new option `app.Suggest` enables command and flag suggestions via
the jaro-winkler distance algorithm. Flags are scoped to their
appropriate commands whereas command suggestions are scoped to the
current command level.

Signed-off-by: Sascha Grunert <sgrunert@suse.com>
coilysiren
coilysiren previously approved these changes Mar 5, 2020
@stale
Copy link

stale bot commented Jun 3, 2020

This issue or PR has been automatically marked as stale because it has not had recent activity. Please add a comment bumping this if you're still interested in it's resolution! Thanks for your help, please let us know if you need anything else.

@stale stale bot added the status/stale stale due to the age of it's last update label Jun 3, 2020
@stale
Copy link

stale bot commented Jul 3, 2020

Closing this as it has become stale.

@stale stale bot closed this Jul 3, 2020
@mfinley3
Copy link

Hey, just curious what ended up happening with this feature. Looks like stale bot closed it but seems like the work was good. Getting suggestions on invalid commands would still be a nice feature to have

@hojongs
Copy link

hojongs commented Mar 4, 2022

Hey, Why don't you merge this feature?

@meatballhat meatballhat reopened this Apr 19, 2022
@meatballhat meatballhat removed the status/stale stale due to the age of it's last update label Apr 21, 2022
@meatballhat meatballhat added this to the Release 2.5.0 milestone Apr 21, 2022
@meatballhat meatballhat changed the base branch from master to main April 21, 2022 22:19
@meatballhat meatballhat added kind/feature describes a code enhancement / feature request area/v2 relates to / is being considered for v2 labels Apr 21, 2022
@meatballhat meatballhat added the status/superseded has been superseded label May 7, 2022
meatballhat added a commit that referenced this pull request May 17, 2022
@meatballhat meatballhat merged commit 3d67b75 into urfave:main May 17, 2022
@renovate renovate bot mentioned this pull request Feb 19, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/v2 relates to / is being considered for v2 kind/feature describes a code enhancement / feature request status/superseded has been superseded
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Did you mean" hint on wrong command or flag
7 participants