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

Suggest language when one isn't found #116

Merged
merged 7 commits into from
Nov 8, 2018
Merged

Suggest language when one isn't found #116

merged 7 commits into from
Nov 8, 2018

Conversation

stevenzeck
Copy link
Contributor

@stevenzeck stevenzeck commented Nov 6, 2018

Closes #115

  • Suggests a language if the provided language -l is not found

const langs = unibeautify.findLanguages({ name: language });
if (langs.length === 0) {
const bestMatchLanguage = getAllLanguages().find(lang => {
return lang.toLowerCase() === language.toLowerCase();
Copy link
Member

Choose a reason for hiding this comment

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

I would be OK if you also wanted to add https://www.npmjs.com/package/fast-levenshtein

cc @lassik @muuvmuuv

Something like:

const languages = getAllLanguages();

const distances: number[] = languages.map(lang =>
  levenshtein.get(lang.toLowerCase(), language.toLowerCase())
);
const bestDistance: number = Math.min(...distances);

const bestMatchLanguages = getAllLanguages().filter((lang, index) =>
  distances[index] === bestDistance
);

// ...

const error = new Error(
  `Language '${language}' was not found. Did you mean '${bestMatchLanguages.join(" or ")}'?`
);

The above only gets the best. It could also be modified to support an arbitrary threshold:

- distances[index] === bestDistance
+ const distanceThreshold = 2;
+ distances[index] <= (bestDistance + distanceThreshold)

So C would also show C++ and visa versa 😉 .

return this.handleError(error, 1);
}
}
return Promise.resolve({});
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be return Promise.resolve(); and the method return type is : Promise<void>

test/commands/BeautifyCommand.spec.ts Outdated Show resolved Hide resolved
const error = new Error(
`Language '${language}' was not found. Did you mean '${bestMatchLanguage}'?`
`Language '${language}' was not found. Did you mean:\n\n${bestMatchLanguages.map(lang => `- ${lang}`).join("\n")}`
Copy link
Member

Choose a reason for hiding this comment

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

We should probably limit the number of matches returned, just in case.

const limit = 3;
bestMatchLanguages.slice(0, limit).map(lang => `- ${lang}`).join("\n")

if (bestMatchLanguages.length > 0) {
const limit = 3;
const error = new Error(
`Language '${language}' was not found. Did you mean:\n\n${bestMatchLanguages
Copy link
Member

Choose a reason for hiding this comment

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

I don't think I like the double \n here.

I just noticed the snapshot at https://github.com/Unibeautify/unibeautify-cli/pull/116/files#diff-79f3cbac345f885b9f1a9467fa09ae86R55

Suggested change
`Language '${language}' was not found. Did you mean:\n\n${bestMatchLanguages
`Language '${language}' was not found. Did you mean:\n${bestMatchLanguages

const error = new Error(
`Language '${language}' was not found. Did you mean:\n\n${bestMatchLanguages
.slice(0, limit)
.map(lang => `- ${lang}`)
Copy link
Member

Choose a reason for hiding this comment

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

One thing I like to do is indent list items to the console.

Suggested change
.map(lang => `- ${lang}`)
.map(lang => ` - ${lang}`)

@Glavin001
Copy link
Member

@stevenzeck I think this is getting really close. Awesome work! I'm really excited to merge this 😃

@Glavin001 Glavin001 merged commit 5e04f0c into master Nov 8, 2018
@stevenzeck stevenzeck deleted the suggest-language branch December 13, 2018 23:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants