search

metrumresearchgroup / bbr

R interface for model and project management
https://metrumresearchgroup.github.io/bbr/
Other
23 stars 2 forks source link

Silence dplyr 1.1.0 warnings #578

Closed kyleam closed 1 year ago

kyleam commented 1 year ago

With the latest dplyr release, our test suite triggers a good number of warnings. This PR resolves them.

devtools::test() result summary before this PR:

[ FAIL 0 | WARN 17 | SKIP 0 | PASS 1115 ]

devtools::test() result summary after this PR:

[ FAIL 0 | WARN 0 | SKIP 0 | PASS 1115 ]

@barrettk there's no rush on this (just would be good to get in before the next release), but I'd especially appreciate your input on the delete_models change. I pretty sure multiple matches are expected, but I was getting turned around when trying to follow the code (in particular I was confused by the pasting of tags).

kyleam commented 1 year ago

I feel like we may have some of these elsewhere that we could consolidate here, but may not be worth the effort to do anything more than a cursory look for them.

Good point. A quick grep for packageVersion and "minimum" just turned up the sites below, so I'm not spotting any good candidates for moving at this point. The unmerged use_bbi pr (gh-579) introduces a kludge that could be reworked to move to compat.R. I lean toward keeping that local in use_bbi, though I'm not sure I'm being consistent in weighing "if used in one spot, nice to keep local" vs "nice to have compat kludges in one spot".

https://github.com/metrumresearchgroup/bbr/blob/9ee2d59e80fc4618aaa825d1b003a3e77c8f998e/tests/testthat/test-workflow-bbi.R#L165-L168

https://github.com/metrumresearchgroup/bbr/blob/9ee2d59e80fc4618aaa825d1b003a3e77c8f998e/R/utils.R#L357-L360

seth127 commented 1 year ago

I lean toward keeping that local in use_bbi, though I'm not sure I'm being consistent in weighing "if used in one spot, nice to keep local" vs "nice to have compat kludges in one spot".

That makes sense to me. Thanks for checking.