Skip to content

Migrate to base R pipe (|>) and drop magrittr from Imports - #26

Merged
eboyer221 merged 1 commit into
mainfrom
dev-base-r-pipe
Jun 1, 2026
Merged

Migrate to base R pipe (|>) and drop magrittr from Imports#26
eboyer221 merged 1 commit into
mainfrom
dev-base-r-pipe

Conversation

@eboyer221

Copy link
Copy Markdown
Contributor

Follow-up to #25 per @AbhirupaGhosh's suggestion to use the base R pipe.

Changes

  • Replaced all %>% with |> across R/app.R and R/utils.R.
  • Removed magrittr from DESCRIPTION Imports (it was only there to enable %>%).
  • Removed @importFrom magrittr %>% from R/app.R.
  • Regenerated NAMESPACE via devtools::document(), which drops the now-unused importFrom(magrittr,"%>%") line.

Why

  • One fewer dependency for users and Bioconductor CI to install, consistent with Bioconductor's dependency-minimization guidance.
  • |> has been the standard R forward pipe since R 4.1 (2021); DESCRIPTION already requires R >= 4.5.0, so no version compatibility concern.

Test plan

  1. devtools::load_all() then launchAMRDashboard() — dashboard builds and tabs render normally.
  2. grep -rn "%>%" R/ returns no hits.
  3. grep -n "magrittr" DESCRIPTION NAMESPACE R/ returns no hits.

@amcim amcim left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The changes were made, everything ran, and the primary concern of this switch (which is using a "." placeholder) is dealt with, so approved for merge

@eboyer221
eboyer221 merged commit 073d39d into main Jun 1, 2026
7 of 8 checks passed
@eboyer221
eboyer221 deleted the dev-base-r-pipe branch June 1, 2026 21:03
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.

2 participants