-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Remove rapidfuzz version pin #2730
Conversation
setup.cfg
Outdated
@@ -98,7 +98,7 @@ install_requires = | |||
elastic-apm | |||
|
|||
# context matching | |||
rapidfuzz==2.0.13 | |||
rapidfuzz!=2.0.14 |
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.
I would pin it more explicitly, something like rapidfuzz>=2.0.15,<2.1
.
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.
@masci why "pinning" the minor version with the <2.1
restriction in this particular case? I wouldn't expect any breaking changes from a minor version increase. I see the disadvantage that people using haystack and rapidfuzz (for something else using some new 2.1 feature) together in their app wouldn't be able to do so.
To give a bit more context to the situation: till now any version from 2.0.7 to 2.0.15 (except 2.0.14) worked. Versions below 2.0.7 weren't tested.
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.
I mistakenly thought this was a dev dependency, if users might have the library already in their project before adding Haystack my proposal is definitely too aggressive. If you think rapidfuzz>=2.0.15,<3
is still too restrictive we can go with rapidfuzz>=2.0.7,<3,!=2.0.14
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.
LGTM! 👍
* remove rapidfuzz version pin * exclude malicious version 2.0.14 * update rapidfuzz version restrictions
Related Issue(s):
dest_start
anddest_end
ofpartial_ratio_alignment
's result can be negative since 2.0.14 rapidfuzz/RapidFuzz#231 (actual dependency issue)Proposed changes:
dest_start
anddest_end
ofpartial_ratio_alignment
's result can be negative since 2.0.14 rapidfuzz/RapidFuzz#231 in Version 2.0.15 we are able to release the pin on rapidfuzz dependency