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

Replace purrr::when() with if() equivalents #1066

Merged
merged 3 commits into from
Nov 23, 2022
Merged

Conversation

IndrajeetPatil
Copy link
Collaborator

@IndrajeetPatil IndrajeetPatil commented Nov 23, 2022

This function will be deprecated in the next (1.0.0) {purrr} release (https://github.com/tidyverse/purrr/blob/55c9a8ab8788d878ce9e8e80b867139e46d15395/R/deprec-when.R#L57), and so we should remove its usage to prevent user-facing deprecation messages.

This function will be deprecated in the next `{purrr}` release (https://github.com/tidyverse/purrr/blob/55c9a8ab8788d878ce9e8e80b867139e46d15395/R/deprec-when.R#L57), and so we should port it over and prevent user-facing deprecation messages.
@github-actions
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if be75da3 is merged into main:

  •   :ballot_box_with_check:cache_applying: 37.4ms -> 36.2ms [-10.18%, +3.63%]
  •   :ballot_box_with_check:cache_recording: 1.14s -> 1.14s [-1.47%, +0.7%]
  •   :ballot_box_with_check:without_cache: 2.98s -> 2.97s [-1.62%, +0.85%]

Further explanation regarding interpretation and methodology can be found in the documentation.

@lorenzwalthert
Copy link
Collaborator

Thanks. I am not sure we should port it since it requires maintenance. Can't we just use if, as the docs suggest? Also, when does this require a new release on CRAN to avoid a user facing warning?

@IndrajeetPatil IndrajeetPatil changed the title Port over purrr::when() Replace purrr::when() with if() equivalents Nov 23, 2022
@IndrajeetPatil
Copy link
Collaborator Author

Can't we just use if, as the docs suggest?

Done. No longer porting over the function.

Also, when does this require a new release on CRAN to avoid a user facing warning?

IIRC, {purrr} is scheduled for CRAN submission on the 15th of December.

@codecov-commenter
Copy link

Codecov Report

Merging #1066 (fd531d8) into main (be75da3) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1066   +/-   ##
=======================================
  Coverage   91.06%   91.06%           
=======================================
  Files          46       46           
  Lines        2685     2685           
=======================================
  Hits         2445     2445           
  Misses        240      240           
Impacted Files Coverage Δ
R/nest.R 100.00% <100.00%> (ø)
R/rules-tokens.R 100.00% <100.00%> (ø)
R/transform-files.R 97.41% <100.00%> (+0.01%) ⬆️

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

@github-actions
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if be75da3 is merged into main:

  •   :ballot_box_with_check:cache_applying: 24.1ms -> 24.7ms [-2.65%, +7.51%]
  •   :rocket:cache_recording: 745ms -> 740ms [-1.19%, -0.25%]
  •   :rocket:without_cache: 1.87s -> 1.84s [-2.04%, -0.97%]

Further explanation regarding interpretation and methodology can be found in the documentation.

@lorenzwalthert
Copy link
Collaborator

IIRC, {purrr} is scheduled for CRAN submission on the 15th of December.

Should we release a new version before that?

@IndrajeetPatil IndrajeetPatil merged commit 55e8559 into main Nov 23, 2022
@IndrajeetPatil IndrajeetPatil deleted the port_over_when branch November 23, 2022 14:34
@IndrajeetPatil
Copy link
Collaborator Author

Should we release a new version before that?

Yes, I think that would be a good idea. Maybe at the end of this month?

Btw, can I upgrade myself from ctb to aut for the next release?
I don't know if you have a particular milestone in mind when this should happen.

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.

None yet

3 participants