UrbanAnalyst / dodgr

Distances on Directed Graphs in R
https://urbananalyst.github.io/dodgr/
127 stars 16 forks source link

Use `expect_no_message()` over `expect_silent()` #198

Closed DavisVaughan closed 1 year ago

DavisVaughan commented 1 year ago

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 of yours that was expecting no output. I've done that by swapping expect_silent() for expect_no_message(), which I'm guessing is what you were trying to avoid.

We plan to submit dplyr 1.1.0 on January 27th.

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[bot] commented 1 year ago

Codecov Report

Merging #198 (49f44ee) into main (84ee85b) will decrease coverage by 0.01%. The diff coverage is n/a.

:exclamation: Current head 49f44ee differs from pull request most recent head 9697431. Consider uploading reports for the commit 9697431 to get more accurate results

@@            Coverage Diff             @@
##             main     #198      +/-   ##
==========================================
- Coverage   94.39%   94.37%   -0.02%     
==========================================
  Files          47       47              
  Lines        5580     5580              
==========================================
- Hits         5267     5266       -1     
- Misses        313      314       +1     
Impacted Files Coverage Δ
src/match-points.cpp 97.67% <0.00%> (-1.17%) :arrow_down:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

mpadge commented 1 year ago

Thank you very much for the attention and care @DavisVaughan. I'll merge straight away. This package was only recently updated on CRAN, so I dare not try another one so soon. But I'll make sure that i do it before 27th Jan. Thanks!