-
Notifications
You must be signed in to change notification settings - Fork 124
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
fix: Avoid stray argument #2054
Conversation
R/brdg_hierarchical.R
Outdated
@@ -308,7 +308,6 @@ pier_summary_hierarchical <- function(cards, | |||
# this ensures the correct order when there are 10+ groups |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@edelarua can you take a look at this?
- I think we may just need to replace the preceding
|>
with%>%
to put the input data frame in thte second argument. - If we don't already have a test for AE tables with 10+ by levels, can we add one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With both |>
or %>%
, the LHS comes in as the first argument, and cards_no_attr |> ...
as the second. The dot comes in third, and is mapped to copy
, which currently is never touched in dplyr and hence never errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, right, I forgot about that. Can we use y = _
with the base pipe too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that would work too!
@edelarua let's use %>%
so we can support R 4.1, since the base R placeholder was introduced in R 4.2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about storing the RHS in a variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Closes #2050.