thisisnic commented on PR #13786: URL: https://github.com/apache/arrow/pull/13786#issuecomment-1231628515
> This is so close! I'm definitely in favor of deferring `filter()` and `summarise()` to another PR (although it will be confusing if they are not implemented before the next release). I really like how you split up the functions in dplyr-across.R...made it easy to review! > > The _one_ thing I think this needs is a set of tests directly on `expand_across()`. I think you can very easily convert the existing tests to something like: > > ```r > library(rlang, warn.conflicts = FALSE) > library(arrow, warn.conflicts = FALSE) > #> Some features are not enabled in this build of Arrow. Run `arrow_info()` for more information. > > testthat::expect_identical( > arrow:::expand_across( > data.frame(dbl = double(), dbl2 = double()), > rlang::quos(across(c(dbl, dbl2), round)) > ), > list( > dbl = quo(round(dbl)), > dbl2 = quo(round(dbl2)) > ) > ) > ``` > > Created on 2022-08-26 by the [reprex package](https://reprex.tidyverse.org) (v2.0.1) > > (With apologies for not suggesting that in my first review! I think those tests would help to communicate exactly what `expand_across()` is doing and highlight the fact that you did a really good job separating the implementation into highly testable pieces.) @paleolimbot Thanks for the review! Quick question for you; I want to keep in the tests which have `across()` inside of `mutate()` as it makes it a lot easier to see "how are we matching dplyr's functionality" which I imagine will come up if when there are bug reports about this. Any tips as to which functionality to include in the `expand_across()` tests vs. the `mutate(across(...)` tests? Or is this going to be a pain, and I should just swap them all over? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
