-
Notifications
You must be signed in to change notification settings - Fork 682
feat: improved user experience for detached instances with prefix matching and suggestions #4199
base: develop
Are you sure you want to change the base?
Conversation
This will have more thorough testing added after #3735 is merged |
maybe a bit late for a pitch, but https://github.com/leeoniya/uFuzzy#a-biased-appraisal-of-similar-work |
Thanks for the suggestion! I intentionally didn't introduce a library for this feature, in an effort to limit the impact on the bundle size. In addition, I'm not sure this is a good fit for your library, based on your comment "match a relatively short search phrase (needle) against a large list of short-to-medium phrases (haystack)" whereas this use case is a very short search phrase against a small list of very short phrases. I'll keep it in mind for future work though. |
640472d
to
8b2c81d
Compare
…here's a single prefix match, or suggest close matches
…th L-diggity if we already have enough prefix matches. Refine distinction between 'match' and 'instance'.
8b2c81d
to
e805106
Compare
Deploying with Cloudflare Pages
|
src/packages/cli/src/detach.ts
Outdated
@@ -19,6 +19,8 @@ export type DetachedInstance = { | |||
version: string; | |||
}; | |||
|
|||
const MAX_SUGGESTIONS = 4; | |||
const MAX_LEVENSHTEIN_DISTANCE = 10; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a huge distance. I'm unsure about how to tune this.
I'd rather we give false positives, than false negatives - if I forget the name of the instance, and get something that's truly wrong, I'd still like a recommendation.
I'm open to suggestions on this. At the same time, it's something that we can tune later, and hopefully we'll get some user feedback on the experience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed. we can tune it later.
src/packages/cli/src/detach.ts
Outdated
@@ -19,6 +19,8 @@ export type DetachedInstance = { | |||
version: string; | |||
}; | |||
|
|||
const MAX_SUGGESTIONS = 4; | |||
const MAX_LEVENSHTEIN_DISTANCE = 10; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed. we can tune it later.
src/packages/cli/src/detach.ts
Outdated
* @returns {{ match: string } | { suggestions: string[] }} an object | ||
* containiner either a single exact `match` or a number of `suggestions` | ||
*/ | ||
async function getSimilarInstanceNames( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh man I really want to optimize this function! haha. But since the actual number of files is typically going to be on the order of 1
I'll hold back! haha
p.s., using our findInsertPosition
util function is always a good time.
src/packages/cli/src/cli.ts
Outdated
@@ -181,7 +192,8 @@ if (argv.action === "start") { | |||
}) | |||
.catch(err => { | |||
// the child process would have output its error to stdout, so no need to | |||
// output anything more | |||
// do anything more other than set the exitCode | |||
process.exitCode = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
smart!
src/packages/cli/src/cli.ts
Outdated
console.log(`${porscheColor(instanceName)} not found.`); | ||
if (instanceOrSuggestions.suggestions?.length > 0) { | ||
console.log(); | ||
console.log("Did you mean:"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/packages/cli/src/detach.ts
Outdated
* | ||
* @param {string} a - The first string to compare. | ||
* @param {string} b - The second string to compare. | ||
* @return {number} The Levenshtein distance between the two strings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I've mentioned this before, but including the types in these definitions are kinda redundant. I don't think you need to remove them, but I don't think you need to add them in the first place either.
Detached instances are pretty awesome, but they have long names that you have to type all the time!
With fuzzy matching (and some helpful suggestions), you can get by without typing so much:
note: now you only need to enter the unique prefix of the instance name. Ganache will use this prefix to identify the instance.