paleolimbot commented on code in PR #13160:
URL: https://github.com/apache/arrow/pull/13160#discussion_r916828419
##########
r/tests/testthat/test-dplyr-summarize.R:
##########
@@ -999,3 +999,12 @@ test_that("summarise() can handle scalars and literal
values", {
tibble(y = 2L)
)
})
+
Review Comment:
Also add a test for a nested namespaced thing like `sum(base::log(x))` or
`base::sum(x) + 1` (I'm sure they work but I seem to remember that the nesting
added some complexity to the summarise evaluation).
##########
r/R/util.R:
##########
@@ -93,6 +93,13 @@ all_funs <- function(expr) {
expr <- quo_get_expr(expr)
}
names <- all.names(expr)
+ # if we have namespace-qualified functions rebuild the function name with the
+ # pkg:: prefix
+ for (i in seq_along(names)) {
+ if (names[i] == "::") {
+ names[i] <- paste0(names[i+1], names[i], names[i+2])
+ }
+ }
Review Comment:
This is very cool and simpler than what I had been thinking!
Could you add tests (maybe in test-util.R) to make sure this works? (It
does! But the tests would have helped me make sure of that without having to
pull the PR and check). These are the pieces I checked but maybe a few variants
without namespaced functions would be good too.
``` r
arrow:::all_funs(rlang::quo(pkg::fun()))
#> [1] "pkg::fun"
arrow:::all_funs(rlang::quo(pkg::fun(other_pkg::obj)))
#> [1] "pkg::fun"
arrow:::all_funs(rlang::quo(other_fun(pkg::fun())))
#> [1] "other_fun" "pkg::fun"
arrow:::all_funs(rlang::quo(other_pkg::other_fun(pkg::fun())))
#> [1] "other_pkg::other_fun" "pkg::fun"
```
<sup>Created on 2022-07-08 by the [reprex
package](https://reprex.tidyverse.org) (v2.0.1)</sup>
--
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]
