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.

Sign up for GitHub

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

Jump to bottom

faster levenshtein #2403

Closed
wants to merge 1 commit into from
Closed

faster levenshtein #2403

wants to merge 1 commit into from

Conversation

ka-weihe
Copy link

leven is one of the slowest levenshtein packages out there and the author is trying to deceive people with shitty benchmarks where he is only comparing it slower packages. Please consider using fastest-levenshtein instead which is 10x faster and much higher quality.

@ka-weihe ka-weihe requested a review from a team as a code owner December 22, 2020 17:33
@darcyclarke darcyclarke added Release 7.x work is associated with a specific npm 7 release release: next These items should be addressed in the next release Needs Review semver:patch semver patch level for changes and removed release: next These items should be addressed in the next release labels Dec 30, 2020
@darcyclarke
Copy link
Contributor

darcyclarke commented Feb 13, 2021 •
edited

Appreciate this change. We'll definitely look at this in the future but we're actually very happy w/ & prefer using deps from collaborators we've worked within different areas of the project (ex. educes the scope of maintainers we'd have to poke if any transitive dep required updating). The stats on fastest-levenshtein look amazing, so we'll definitely circle back on this. Closing for now as there isn't a path forward.

@wraithgar wraithgar reopened this Aug 11, 2021
@wraithgar
Copy link
Member

Revisiting this since leven dropped node10 support among other things.

@wraithgar wraithgar assigned wraithgar and unassigned isaacs and nlf Aug 11, 2021
@wraithgar
Copy link
Member

wraithgar commented Aug 11, 2021 •
edited

CI tests in node 10 and above https://travis-ci.org/github/ka-weihe/fastest-levenshtein
MIT https://github.com/ka-weihe/fastest-levenshtein/blob/master/LICENSE.md
No subdependencies.

@wraithgar wraithgar changed the base branch from latest to release-next August 11, 2021 17:49
wraithgar added a commit that referenced this pull request Aug 11, 2021
@wraithgar
fix(did-you-mean): switch levenshtein libraries
d49029f 
`leven` dropped support for node10 and we still currently have to support
it.  Moved to https://github.com/ka-weihe/fastest-levenshtein

Originally discussed in #2403, but the
did-you-mean lib moved quite a bit since then and there were conflicts
so I made a new PR
@wraithgar wraithgar mentioned this pull request Aug 11, 2021
Merged
@wraithgar
Copy link
Member

new PR w/o conflicts here #3640

wraithgar added a commit that referenced this pull request Aug 11, 2021
@wraithgar
fix(did-you-mean): switch levenshtein libraries
eda148f 
`leven` dropped support for node10 and we still currently have to support
it.  Moved to https://github.com/ka-weihe/fastest-levenshtein

Originally discussed in #2403, but the
did-you-mean lib moved quite a bit since then and there were conflicts
so I made a new PR
@wraithgar
Copy link
Member

@ka-weihe sorry for the noise here, closing this again in favor of the new PR. Changing deps is usually something we do ourselves since it's more than just running npm install etc. The bundled deps have to be updated for instance. Discussion can continue in the new PR.

@wraithgar wraithgar closed this Aug 11, 2021
wraithgar added a commit that referenced this pull request Aug 17, 2021
@wraithgar
fix(did-you-mean): switch levenshtein libraries
32e88c9 
`leven` dropped support for node10 and we still currently have to support
it.  Moved to https://github.com/ka-weihe/fastest-levenshtein

Originally discussed in #2403, but the
did-you-mean lib moved quite a bit since then and there were conflicts
so I made a new PR

PR-URL: #3640
Credit: @wraithgar
Close: #3640
Reviewed-by: @nlf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@wraithgar wraithgar

Labels
Release 7.x work is associated with a specific npm 7 release semver:patch semver patch level for changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ka-weihe @darcyclarke @wraithgar @isaacs @nlf