-
Notifications
You must be signed in to change notification settings - Fork 679
feat: improved user experience for detached instances with prefix matching and suggestions #4199
base: develop
Are you sure you want to change the base?
Changes from 6 commits
a3fe8a7
bb413f6
1fe3dd7
65381c0
18ab2c5
e805106
4ce7169
f8e29a8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -164,11 +164,22 @@ if (argv.action === "start") { | |
} else if (argv.action === "stop") { | ||
const instanceName = argv.name; | ||
|
||
stopDetachedInstance(instanceName).then(instanceFound => { | ||
if (instanceFound) { | ||
console.log("Instance stopped"); | ||
stopDetachedInstance(instanceName).then(instanceOrSuggestions => { | ||
if ("instance" in instanceOrSuggestions) { | ||
const highlightedName = porscheColor(instanceOrSuggestions.instance.name); | ||
console.log(`${highlightedName} stopped.`); | ||
} else { | ||
console.error("Instance not found"); | ||
process.exitCode = 1; | ||
console.log(`${porscheColor(instanceName)} not found.`); | ||
if (instanceOrSuggestions.suggestions?.length > 0) { | ||
console.log(); | ||
console.log("Did you mean:"); | ||
console.log( | ||
instanceOrSuggestions.suggestions | ||
.map(name => " - " + porscheColor(name)) | ||
.join("\n") | ||
); | ||
} | ||
} | ||
}); | ||
} else if (argv.action === "start-detached") { | ||
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. smart! |
||
}); | ||
} else if (argv.action === "list") { | ||
getDetachedInstances().then(instances => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. agreed. we can tune it later. |
||
const FILE_ENCODING = "utf8"; | ||
const START_ERROR = | ||
"An error occurred spawning a detached instance of Ganache:"; | ||
|
@@ -53,6 +55,12 @@ export async function removeDetachedInstanceFile( | |
return false; | ||
} | ||
|
||
// A fuzzy matched detached instance(s). Either a strong match as instance, | ||
// or a list of suggestions. | ||
type InstanceOrSuggestions = | ||
| { instance: DetachedInstance } | ||
| { suggestions: string[] }; | ||
|
||
/** | ||
* Attempts to stop a detached instance with the specified instance name by | ||
* sending a SIGTERM signal. Returns a boolean indicating whether the process | ||
|
@@ -61,25 +69,127 @@ export async function removeDetachedInstanceFile( | |
* | ||
* Note: This does not guarantee that the instance actually stops. | ||
* @param {string} instanceName | ||
* @returns boolean indicating whether the instance was found. | ||
* @returns {InstanceOrSuggestions} an object containing either the stopped | ||
* `instance`, or `suggestions` for similar instance names | ||
*/ | ||
export async function stopDetachedInstance( | ||
instanceName: string | ||
): Promise<boolean> { | ||
): Promise<InstanceOrSuggestions> { | ||
let instance; | ||
|
||
try { | ||
// getDetachedInstanceByName() throws if the instance file is not found or | ||
// cannot be parsed | ||
const instance = await getDetachedInstanceByName(instanceName); | ||
instance = await getDetachedInstanceByName(instanceName); | ||
} catch { | ||
const similarInstances = await getSimilarInstanceNames(instanceName); | ||
|
||
if ("match" in similarInstances) { | ||
try { | ||
instance = await getDetachedInstanceByName(similarInstances.match); | ||
} catch (err) { | ||
if ((err as NodeJS.ErrnoException).code === "ENOENT") { | ||
// The instance file was removed between the call to | ||
// `getSimilarInstanceNames` and `getDetachedInstancesByName`, but we | ||
// didn't get suggestions (although some may exist). We _could_ | ||
// reiterate stopDetachedInstance but that seems messy. Let's just | ||
// tell the user the instance wasn't found, and be done with it. | ||
return { | ||
suggestions: [] | ||
}; | ||
} | ||
throw err; | ||
} | ||
} else { | ||
return { suggestions: similarInstances.suggestions }; | ||
} | ||
} | ||
|
||
if (instance) { | ||
// process.kill() throws if the process was not found (or was a group | ||
// process in Windows) | ||
process.kill(instance.pid, "SIGTERM"); | ||
try { | ||
process.kill(instance.pid, "SIGTERM"); | ||
} catch (err) { | ||
// process not found | ||
// todo: log message saying that the process could not be found | ||
} finally { | ||
await removeDetachedInstanceFile(instance.name); | ||
return { instance }; | ||
} | ||
} | ||
} | ||
|
||
/** | ||
* Find instances with names similar to `instanceName`. | ||
* | ||
* If there is a single instance with an exact prefix match, it is returned as | ||
* the `match` property in the result. Otherwise, up to `MAX_SUGGESTIONS` names | ||
* that are similar to `instanceName` are returned as `suggestions`. Names with | ||
* an exact prefix match are prioritized, followed by increasing Levenshtein | ||
* distance, up to a maximum distance of `MAX_LEVENSHTEIN_DISTANCE`. | ||
* @param {string} instanceName the name for which similarly named instance will | ||
* be searched | ||
* @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 commentThe 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 p.s., using our |
||
instanceName: string | ||
): Promise<{ match: string } | { suggestions: string[] }> { | ||
const filenames: string[] = []; | ||
try { | ||
const parsedPaths = ( | ||
await fsPromises.readdir(dataPath, { withFileTypes: true }) | ||
).map(file => path.parse(file.name)); | ||
|
||
for (const { ext, name } of parsedPaths) { | ||
if (ext === ".json") { | ||
filenames.push(name); | ||
} | ||
} | ||
} catch (err) { | ||
return false; | ||
} finally { | ||
await removeDetachedInstanceFile(instanceName); | ||
if ((err as NodeJS.ErrnoException).code === "ENOENT") { | ||
// instances directory does not exist, so there can be no suggestions | ||
return { suggestions: [] }; | ||
} | ||
} | ||
|
||
const prefixMatches = []; | ||
for (const name of filenames) { | ||
if (name.startsWith(instanceName)) { | ||
davidmurdoch marked this conversation as resolved.
Show resolved
Hide resolved
|
||
prefixMatches.push(name); | ||
} | ||
} | ||
|
||
if (prefixMatches.length === 1) { | ||
return { match: prefixMatches[0] }; | ||
} | ||
|
||
let suggestions: string[]; | ||
if (prefixMatches.length >= MAX_SUGGESTIONS) { | ||
suggestions = prefixMatches; | ||
} else { | ||
const similar = []; | ||
|
||
for (const name of filenames) { | ||
if (!prefixMatches.some(m => m === name)) { | ||
const distance = levenshteinDistance(instanceName, name); | ||
if (distance <= MAX_LEVENSHTEIN_DISTANCE) { | ||
similar.push({ | ||
name, | ||
distance | ||
}); | ||
} | ||
} | ||
} | ||
similar.sort((a, b) => a.distance - b.distance); | ||
|
||
suggestions = similar.map(s => s.name); | ||
// matches should be at the start of the suggestions array | ||
suggestions.splice(0, 0, ...prefixMatches); | ||
} | ||
return true; | ||
|
||
return { | ||
suggestions: suggestions.slice(0, MAX_SUGGESTIONS) | ||
}; | ||
} | ||
|
||
/** | ||
|
@@ -323,3 +433,44 @@ export function formatUptime(ms: number) { | |
|
||
return isFuture ? `In ${duration}` : duration; | ||
} | ||
|
||
/** | ||
* This function calculates the Levenshtein distance between two strings. | ||
* Levenshtein distance is a measure of the difference between two strings, | ||
* defined as the minimum number of edits (insertions, deletions or substitutions) | ||
* required to transform one string into another. | ||
* | ||
* @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 commentThe 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. |
||
*/ | ||
export function levenshteinDistance(a: string, b: string): number { | ||
jeffsmale90 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (a.length === 0) return b.length; | ||
if (b.length === 0) return a.length; | ||
|
||
let matrix = []; | ||
|
||
for (let i = 0; i <= b.length; i++) { | ||
matrix[i] = [i]; | ||
} | ||
|
||
for (let j = 0; j <= a.length; j++) { | ||
matrix[0][j] = j; | ||
} | ||
|
||
for (let i = 1; i <= b.length; i++) { | ||
for (let j = 1; j <= a.length; j++) { | ||
if (b.charAt(i - 1) == a.charAt(j - 1)) { | ||
matrix[i][j] = matrix[i - 1][j - 1]; | ||
} else { | ||
matrix[i][j] = Math.min( | ||
matrix[i - 1][j - 1] + 1, | ||
matrix[i][j - 1] + 1, | ||
matrix[i - 1][j] + 1 | ||
); | ||
} | ||
} | ||
} | ||
|
||
return matrix[b.length][a.length]; | ||
} |
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.
Since this isn't interactive (oh that'd be cool!), and the user can't actually answer the question. Should we use a different phrase here? git uses
The most similar command is
.