Better handle non-existent migration line + suggestions with Levenshtein distance #558
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
brandur
force-pushed
the
brandur-better-handle-non-existent-line
branch
2 times, most recently
from
034c94d
to
052c603
Compare
brandur
force-pushed
the
brandur-better-handle-non-existent-line
branch
from
052c603
to
b471ce5
Compare
bgentry
approved these changes
Aug 23, 2024
| @@ -0,0 +1,21 @@ | |||
| The MIT License (MIT) | |||
|
|
|||
| Copyright (c) 2015 Agniva De Sarker | |||
There was a problem hiding this comment.
Trying to confirm if this is the correct copyright holder for this bit, where did this name come from?
There was a problem hiding this comment.
I vendored the code originally from this repo:
https://github.com/agnivade/levenshtein
I'm thinking I may write my own implementation from scratch at some point when I have a bit more time. I ended up rebuilding a lot of the code anyway to modernize and satisfy lint.
There was a problem hiding this comment.
I added a comment pointing to the source repo.
brandur
force-pushed
the
brandur-better-handle-non-existent-line
branch
from
b471ce5
to
6e3c25e
Compare
…ein distance This is related to #546. That was a documentation problem, but there are some code problems that are related. The first is that an unknown migration line is a panic in the CLI. This is a byproduct of the fact that `rivermigrate.New` checks the migration line name, but doesn't return an error, so it'd panic instead. Here, I'm suggesting that we do a small breaking change by having `rivermigrate.New` return a possible error. It's not great, but it's something that'll easy for people to fix, and might help avoid some future panics. An alternative possibility would be to add a new `NewWithError` or something of that nature, but it'd make the API a little uglier. Along with that change, we also bring in a change to suggest migration lines in case of an unknown one using Levenshtein distances. This is aimed at preventing totally unactionable errors in the event of very simple misspellings (e.g. "workflow" versus "workflows"). Cobra actually already has a similar feature built-in for command misspellings. I vendored in this repo [1] as a Levenshtein implementation and did a little code clean up. I don't want to add another Go module dependency for such a simple algorithm (it's just a dozen lines), and I believe their MIT license should be compatible. [1] https://github.com/agnivade/levenshtein/tree/master
brandur
force-pushed
the
brandur-better-handle-non-existent-line
branch
from
6e3c25e
to
18d3571
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is related to #546. That was a documentation problem, but there are
some code problems that are related.
The first is that an unknown migration line is a panic in the CLI. This
is a byproduct of the fact that
rivermigrate.Newchecks the migrationline name, but doesn't return an error, so it'd panic instead.
Here, I'm suggesting that we do a small breaking change by having
rivermigrate.Newreturn a possible error. It's not great, but it'ssomething that'll easy for people to fix, and might help avoid some
future panics. An alternative possibility would be to add a new
NewWithErroror something of that nature, but it'd make the API alittle uglier.
Along with that change, we also bring in a change to suggest migration
lines in case of an unknown one using Levenshtein distances. This is
aimed at preventing totally unactionable errors in the event of very
simple misspellings (e.g. "workflow" versus "workflows"). Cobra actually
already has a similar feature built-in for command misspellings.
I vendored in this repo [1] as a Levenshtein implementation and did a
little code clean up. I don't want to add another Go module dependency
for such a simple algorithm (it's just a dozen lines), and I believe
their MIT license should be compatible.
[1] https://github.com/agnivade/levenshtein/tree/master