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 Jaro-Winkler algorithm usage with an internal function #1893

Merged
merged 3 commits into from
Apr 30, 2024

Conversation

yigithankarabulut
Copy link
Member

What type of PR is this?

  • cleanup

What this PR does / why we need it:

To support the goal of only requiring the Go standard library. We were using the Jaro-Winkler algorithm for the suggestion from the "xrash/smetrics" package. This function was rewritten to be faithful to stdlib.

Which issue(s) this PR fixes:

Fixes #1892

Testing

go test -run=TestJaroWinkler

Release Notes

(REQUIRED)


@yigithankarabulut yigithankarabulut requested a review from a team as a code owner April 30, 2024 00:02
@yigithankarabulut
Copy link
Member Author

Probably the rate limit was reached in the check and failed. The remaining steps were therefore canceled.

@avorima
Copy link
Contributor

avorima commented Apr 30, 2024

Please run go mod tidy to remove the dependency completely

Copy link
Member

@bartekpacia bartekpacia left a comment

Choose a reason for hiding this comment

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

I appreciate the goal to depend on Go's stdlib only :)

@Juneezee Juneezee changed the title Fix:(issue_1892) Replacing the jaro-winkler algorithm usage with an internal function in suggestion file. Replace Jaro-Winkler algorithm usage with an internal function Apr 30, 2024
suggestions.go Outdated
Comment on lines 19 to 20
// jaroDistance is the measure of similarity between two strings. The result is
// 1 for equal strings, and 0 for completely different strings.
Copy link
Member

Choose a reason for hiding this comment

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

I'd add that the result is between 0 or 1. This description at first made me think the output is either 0 or 1, only.

Signed-off-by: Eng Zer Jun <[email protected]>
@Juneezee Juneezee requested a review from meatballhat April 30, 2024 14:31
Copy link
Member

@bartekpacia bartekpacia left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for contributing :)

@meatballhat
Copy link
Member

meatballhat commented Apr 30, 2024

Sorry about the trouble with codecov 😩 I'll get a token set up as soon as I can!

Update: it's already set up, but I'm assuming the token expired (??) Anyway, I updated it and now I'll re-run the failed checks.

Update Update: Uhhhh the token is updated and appears to be in use, and we're still getting rate limit failure. I think the failing step should probably get a continue-on-error: true

Update 3x: #1898

Copy link
Member

@meatballhat meatballhat left a comment

Choose a reason for hiding this comment

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

Lovely, @yigithankarabulut! Thanks so much 🤘🏼

@meatballhat meatballhat merged commit 38003d1 into urfave:main Apr 30, 2024
11 checks passed
@yigithankarabulut
Copy link
Member Author

@meatballhat You can be sure there will be more to come. Let's go!

@bartekpacia
Copy link
Member

Go go go! :D

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.

Vendor usage of github.com/xrash/smetrics for suggestions
5 participants