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

Preemptively use multiple = "all" in left_join() #1

Merged
merged 1 commit into from
Jan 29, 2023

Conversation

DavisVaughan
Copy link
Contributor

This PR makes your package compatible with the next version of dplyr:

The join functions in dplyr (like left_join()) now return a warning by default when a row in x matches multiple rows in y. While this is typical SQL behavior, it is often unexpected during data analysis (many people don't even know it is possible), so we've decided to make this a warning. In dplyr 1.1.0, you silence this warning with multiple = "all". In the meantime, we need to work around a broken test that was expecting a single warning.

I've done this by preemptively adding multiple = "all" to the join function, which doesn't do anything with CRAN dplyr but silences the warning in dev dplyr.

I've also switched you to a snapshot test, which should be more robust for checking this kind of thing (i.e. it won't fail on CRAN if that section of code starts throwing >1 warning all of a sudden, but will fail in your CI and locally).

We actually plan to submit dplyr 1.1.0 today. We typically give you more time to make changes, but we sent out most of these PRs a month ago and missed yours because injurytools made it to CRAN in mid January. Sorry about that!

Copy link
Owner

@lzumeta lzumeta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this PR!

@lzumeta lzumeta merged commit e8a70fe into lzumeta:master Jan 29, 2023
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