Skip to content

Use magrittr pipe exclusively in examples for consistency #517

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

Merged
merged 1 commit into from
Apr 1, 2025

Conversation

jdblischak
Copy link
Collaborator

@jdblischak jdblischak commented Mar 31, 2025

The R CMD build results for R-devel now include the following warning:

  NB: this package now depends on R (>= 4.1.0)
  WARNING: Added dependency on R >= 4.1.0 because package code uses the
  pipe |> or function shorthand \(...) syntax added in R 4.1.0.
  File(s) using such syntax:
    ‘gs_update_ahr.Rd’ ‘to_integer.Rd’

This is because we have started to use the native pipe |> in some of the examples, but the DESCRIPTION continues to only require R >=3.5:

R (>= 3.5.0)

Given that {gsDesign2} depends on dplyr and imports the magrittr pipe (%>%) into the package namespace, I think it makes sense to continue to use the magritter pipe for now:

importFrom(dplyr,"%>%")

At some point we'll want to migrate the entire package to use the native pipe |>, but at this point I don't think this is urgently needed.

Background:

  • {simtrial} has migrated to the native pipe and requires R >= 4.1.0 (Use base pipe operator simtrial#146). This makes more sense since it allowed us to completely drop the dependency on magrittr (since gsDesign2 depends on dplyr, magrittr will continue to be a transitive dependency whether or not we use its pipe in our code)
  • The gsDesign2 README uses the native pipe (Improve readme code example #348). This is fine since this code is not run during installation and thus does not prevent the package from being built and installed with R 3.5 (and therefore R CMD check does not flag this code as problematic)

@jdblischak jdblischak self-assigned this Mar 31, 2025
Copy link
Collaborator

@nanxstats nanxstats left a comment

Choose a reason for hiding this comment

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

I agree with your evaluation

Copy link
Collaborator

@LittleBeannie LittleBeannie left a comment

Choose a reason for hiding this comment

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

Thanks for catching these, @jdblischak!

@LittleBeannie LittleBeannie merged commit 966a8e6 into Merck:main Apr 1, 2025
7 checks passed
@jdblischak jdblischak deleted the consistent-pipe branch April 1, 2025 16:09
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