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

fix: split suggested command from passed command to get varargs #395

Merged
merged 5 commits into from
May 25, 2023

Conversation

WillieRuemmele
Copy link
Contributor

@WillieRuemmele WillieRuemmele commented May 23, 2023

@W-12459598@

goes from

➜  plugin-telemetry git:(main) ✗ 
 ➜  sf confit get target-dev-hub
 ›   Warning: confit get target-dev-hub is not a sf command.
Did you mean config get? [y/n]: y
Error (1): You must provide one or more configuration variables to get. Run "sf config list" to see the configuration variables you've previously set.

to

 ➜  sf confit get target-dev-hub             
 ›   Warning: confit get target-dev-hub is not a sf command.
Did you mean config get? [y/n]: y
Get Config
===============================
| Name           Value  Success 
| ────────────── ────── ─────── 
| target-dev-hub DevHub true    

and with improved accuracy
before

 ➜  sf get confit target-org    
 ›   Warning: get confit target-org is not a sf command.
Did you mean dev configure repo? [y/n]: n

after

 ➜  sf get confit target-org    
 ›   Warning: get confit target-org is not a sf command.
Did you mean config get? [y/n]: n

Copy link
Member

@mshanemc mshanemc left a comment

Choose a reason for hiding this comment

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

I took a stab at that algo, and wrote some extra tests as part of QA.
#396

must-fix is the dep drop for lodash. Everything else, including the PR, is a suggestion.

@@ -1,7 +1,6 @@
import {color} from '@oclif/color'
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: can it use chalk directly instead of oclif/color, to reduce the dep size?

@@ -29,14 +34,16 @@ const hook: Hook.CommandNotFound = async function (opts) {

let response = ''
try {
response = await ux.prompt(`Did you mean ${color.blueBright(readableSuggestion)}? [y/n]`, {timeout: 4900})
response = await ux.prompt(`Did you mean ${color.blueBright(readableSuggestion)}? [y/n]`, {timeout: 10_000})
Copy link
Member

Choose a reason for hiding this comment

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

there's a ux.confirm which returns a boolean so you don't have to compare the response to y (and it handles other "positive" including Y, yes, etc)

src/index.ts Outdated
// an entry at 0 would be a direct match, an entry at 1 would be a single character off, etc.
const index: string[] = []
// eslint-disable-next-line no-return-assign
commandIDs.map(id => (index[Levenshtein.get(cmd, id)] = id))
Copy link
Member

Choose a reason for hiding this comment

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

in SDR, we've used .get(a,b, { useCollator: true}) to make the algo consider case-insensitivity to be closer than "just any one-letter difference". I don't think our CLI cares because we're all lowercase, but other oclif users might care about it.

@mshanemc mshanemc merged commit a32c167 into main May 25, 2023
@mshanemc mshanemc deleted the wr/didYouMeanVararg branch May 25, 2023 14:49
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

Successfully merging this pull request may close these issues.

2 participants