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 for dplyr 1.1.1 #197

Merged
merged 3 commits into from
Mar 6, 2023
Merged

Conversation

DavisVaughan
Copy link
Contributor

We are preparing to release dplyr 1.1.1 and this package popped up in the revdep checks.

We realized that we didn't get the multiple argument quite right the first time around 😞 . It was too aggressive since it warned on both one-to-many and many-to-many joins. In 1.1.1 we've made two improvements:

  • multiple now defaults to "all". The options of NULL, "error", and "warning" are deprecated.
  • relationship is a new argument to add known constraints onto the join procedure, such as "many-to-one", which replaces multiple = "error".

The default of relationship checks to see if there is a many-to-many relationship between the keys of x and y and will warn if one is present. This should be much rarer than what we checked for before, and targets the most dangerous case that we were trying to warn the user about.

You can read all about relationship here tidyverse/dplyr#6753, along with the issues linked there.

Unfortunately it does affect some code here, but I think it does so in a positive way!

  • You can use a non-equi join in set_dates_service() which makes it faster and simpler
  • The inner_join() in get_trip_geometry() no longer needs multiple to be set. It seems like it was doing a one-to-many join before, which we no longer warn on. But we do have to branch on the dplyr version for this one to support CRAN and dev dplyr.

We plan to submit dplyr 1.1.1 in 2-3 weeks.

This should be compatible with both dev and CRAN dplyr. It would help us out if you could go ahead and send a patch version of your package to CRAN ahead of time! Thanks!

@codecov-commenter
Copy link

Codecov Report

Merging #197 (53233fd) into master (bc8cf53) will decrease coverage by 0.10%.
The diff coverage is 85.71%.

❗ Current head 53233fd differs from pull request most recent head 14b2dc8. Consider uploading reports for the commit 14b2dc8 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff             @@
##            master     #197      +/-   ##
===========================================
- Coverage   100.00%   99.90%   -0.10%     
===========================================
  Files           15       15              
  Lines         1062     1057       -5     
===========================================
- Hits          1062     1056       -6     
- Misses           0        1       +1     
Impacted Files Coverage Δ
R/utils.R 100.00% <ø> (ø)
R/spatial.R 98.85% <66.66%> (-1.15%) ⬇️
R/dates.R 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@polettif
Copy link
Contributor

polettif commented Mar 6, 2023

Thank you 👍

This should be compatible with both dev and CRAN dplyr. It would help us out if you could go ahead and send a patch version of your package to CRAN ahead of time! Thanks!

We'll package a release with some other developments but I think it should work out timewise.

@polettif polettif merged commit b1da6cf into r-transit:master Mar 6, 2023
@DavisVaughan
Copy link
Contributor Author

👍 looking like March 13 for the dplyr release

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.

3 participants