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

Make rapidfuzz optional on sys_platform=emscripten #179

Merged
merged 4 commits into from
Aug 20, 2024

Conversation

@michaelweinold
Copy link
Contributor Author

...hmmm, that is annoying. I just added the same condition to my testing package - and the build ran fine there.

@michaelweinold
Copy link
Contributor Author

...must have been the semicolon.

@michaelweinold michaelweinold requested a review from cmutel August 14, 2024 13:34
@michaelweinold
Copy link
Contributor Author

@cmutel, not sure what black magic I need to re-run the checks on this PR. Otherwise ready for review.

@cmutel
Copy link
Member

cmutel commented Aug 15, 2024

I think the review comes before CI, though I don't really understand that... Don't we also need to shortcircuit the calls which use rapidfuzz, or am I missing other commits or PRs? It seems like this would just lead to an import error.

@michaelweinold
Copy link
Contributor Author

Since you answer to my question

Is there already a fallback such that whatever-was-used-before-rapidfuzz is used instead, when rapidfuzz is not available?

Was "Yup", I assumed all those short-circuits would be in place. Maybe I should have had a look at the link you provided...

@cmutel
Copy link
Member

cmutel commented Aug 16, 2024

OK, CI isn't running because the code didn't change, only the build config.

@michaelweinold
Copy link
Contributor Author

Commit that introduced rapidfuzz and DamerauLevenshtein.distance:

Since the string_distance.py file is still present, we could simply have bw2data fall back to it if rapidfuzz is not installed?

@cmutel
Copy link
Member

cmutel commented Aug 19, 2024

Since the string_distance.py file is still present, we could simply have bw2data fall back to it if rapidfuzz is not installed?

Sure, that would work; the API might be slightly different but we can control it in the file we included (i.e. make it fit rapidfuzz).

@michaelweinold
Copy link
Contributor Author

...ok, I think it's all ready - you're already falling back to the string_distance.py file anyway!

@cmutel cmutel merged commit d8d008e into main Aug 20, 2024
@cmutel cmutel deleted the make-rapidfuzz-optional-on-emscripten-sys_platform branch August 20, 2024 09:37
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