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

Replace fast-levenshtein with fastest-levenshtein #130

Merged
merged 4 commits into from
May 24, 2023

Conversation

marc136
Copy link
Contributor

@marc136 marc136 commented May 16, 2023

This PR relates to #82 and removes one direct dependency and updates an indirect one.

fast-levenshtein@3.0.0 currently installs fastest-levenshtein@1.0.12 according to the package-lock.json.
I updated to 1.0.16 because I saw no indication that the compiled typescript output should no longer be compatible to node.js 10.0.0.

Currently, elm-review uses levenshtein.get(flag, f.name) and does not pass the option to use locale-sensitive string comparisons. This means that fast-levenshtein does this:

var levenshtein = require('fastest-levenshtein');
...
var Levenshtein = {
    get: function(str1, str2, options) {
        if (options.collate) { ... }
        return levenshtein.distance(str1, str2);
    }
    ...
}

See https://github.com/hiddentao/fast-levenshtein/blob/master/levenshtein.js#L84

I think it should not create a merge conflict with #126, but I'm not sure.

marc136 added 4 commits May 16, 2023 23:32
elm-review uses `levenshtein.get(flag, f.name)` and does not pass the
option to use locale-sensitive string comparisons. This means that
`fast-levenshtein` does this:

```js
var levenshtein = require('fastest-levenshtein');
...
var Levenshtein = {
    get: function(str1, str2, options) {
        if (options.collate) { ... }
        return levenshtein.distance(str1, str2);
    }
    ...
}
```
See https://github.com/hiddentao/fast-levenshtein/blob/master/levenshtein.js#L84
@jfmengels
Copy link
Owner

Hey @marc136!
Sorry for the delay, I was on a much needed vacation 😅 🌲
Thanks for looking into this! This looks good to me 😊

@jfmengels jfmengels merged commit 7e5b3a0 into jfmengels:master May 24, 2023
@marc136
Copy link
Contributor Author

marc136 commented May 24, 2023

No worries, even with vacation, it was only a week 👟

@lishaduck lishaduck mentioned this pull request Jun 25, 2024
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