-
-
Notifications
You must be signed in to change notification settings - Fork 21
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
chore(fuzzywuzzy): use thefuzz, fix levenshtein #747
Conversation
@@ -40,7 +40,7 @@ Unidecode = ">=1.3.2" | |||
python-dotenv = ">=0.19.0" | |||
frictionless = {extras = ["pandas"], version = ">=4.40.8"} | |||
thefuzz = ">=0.19.0" | |||
python-Levenshtein = ">=0.12.2" | |||
python-Levenshtein = "^0.12.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be useful to add a note explaining why is it pinned and link the issue. This will make it easier to unpin in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, datadiff
doesn't show any changes either.
Thanks, @Marigold! Added a short comment referencing the issue with |
fuzzywuzzy
:thefuzz
. See https://github.com/seatgeek/fuzzywuzzy for more details.thefuzz
in some instances.python-Levenstein
to a specific version. Otherwise,thefuzz
usage will fail, because of a known bug: See NameError: name 'matching_blocks' is not defined seatgeek/thefuzz#43, PyPI package name conflict rapidfuzz/python-Levenshtein#1, replace python-Levenshtein with rapidfuzz seatgeek/thefuzz#10,I noticed this error when trying to run
walkthrough charts
. It would not work, because it relies onthefuzz
.