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

Fix tests for corrected tokenize_skip_ngrams() #106

Merged
merged 1 commit into from
Mar 21, 2018
Merged

Fix tests for corrected tokenize_skip_ngrams() #106

merged 1 commit into from
Mar 21, 2018

Conversation

lmullen
Copy link
Contributor

@lmullen lmullen commented Mar 14, 2018

This pull request fixes two failing tests caused by an imminent release of v0.2 of the tokenizers package. The version 0.1.4 of the tokenizers package currently on CRAN has an incorrect implementation of skip n-grams. (See this issue for details.) The master branch of tokenizers has a fix. As a result the indices used in the tests are now incorrect and I have updated them.

Note that tokenize_skip_ngrams() now returns vastly more tokens, which is how it is supposed to work, but which might come as a surprise.

@lmullen
Copy link
Contributor Author

lmullen commented Mar 14, 2018

The CI checks failed because they are using the CRAN version of tokenizers.

@juliasilge
Copy link
Owner

Thank you so much, Lincoln! 🙌

@juliasilge
Copy link
Owner

Wowwwwwwwww, that is a lot more skip grams. 😯

@juliasilge juliasilge merged commit 517da8e into juliasilge:master Mar 21, 2018
@lmullen
Copy link
Contributor Author

lmullen commented Mar 21, 2018

@juliasilge Thanks for the quick response to the CRAN maintainers, and for your flexibility in having to create a new release because of tokenizers changes.

@juliasilge
Copy link
Owner

@lmullen Do you think the CRAN maintainers intend for me to submit it right away (when my Travis builds and win-builder are still failing because binaries aren't built yet) or to wait until I can demonstrate that everything is passing?

@lmullen
Copy link
Contributor Author

lmullen commented Mar 21, 2018

I think they probably want it right away. They will run their own tests on the current CRAN, I'd guess.

@github-actions
Copy link

This pull request has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex: https://reprex.tidyverse.org) and link to this issue.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants