-
Notifications
You must be signed in to change notification settings - Fork 71
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
Don't require dplyr anywhere #1131
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1131 +/- ##
==========================================
+ Coverage 92.33% 92.35% +0.01%
==========================================
Files 46 46
Lines 2650 2654 +4
==========================================
+ Hits 2447 2451 +4
Misses 203 203
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 8f839e6 is merged into main:
Further explanation regarding interpretation and methodology can be found in the documentation. |
@@ -1,16 +1,16 @@ | |||
test_that("caching utils make right blocks with semi-colon", { | |||
blocks_simple_uncached <- compute_parse_data_nested(c("1 + 1", "2; 1+1")) %>% | |||
dplyr::mutate(is_cached = FALSE) %>% | |||
mutate(is_cached = FALSE) %>% |
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.
where's %>%
coming from if not dplyr?
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.
nvm :)
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.
{magritrr}? We'll at some point transition to |>
if minimal supported R version has it.
R/compat-dplyr.R
Outdated
@@ -57,3 +57,12 @@ map_dfr <- function(.x, .f, ...) { | |||
res <- map(.x, .f, ...) | |||
vec_rbind(!!!res) | |||
} | |||
|
|||
mutate <- function(.data, ...) { |
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.
use base::transform() instead? use case here seems like a drop-in
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.
ok sure.
…nse, !!, etc.) and it's not intended to be used in source code anyways
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 68977f3 is merged into main:
Further explanation regarding interpretation and methodology can be found in the documentation. |
select(token, terminal, text, newlines, spaces) | ||
cols <- c('token', 'terminal', 'text', 'newlines', 'spaces') | ||
pd$child[[1]][, cols] |
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.
maybe subset(pd$child[[1]], select = cols)
?
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.
or since we've magrittr:
pd$child[[1]] %>%
subset(select = c(...))
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.
I don't know... the audience of the vignette is probably R developers who know subset()
but I think it's one of these functions that are not used often anymore these days... ]
is not very pretty here I know but probably easier to understand for most.
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.
LGTM! Comment is optional/up to preference.
Superseeds #1128.