[GitHub] [arrow] paleolimbot commented on a diff in pull request #13160: ARROW-14575: [R] Allow functions with `pkg::` prefixes

[GitBox <gi...@apache.org>]

paleolimbot commented on code in PR #13160:
URL: https://github.com/apache/arrow/pull/13160#discussion_r914803637


##########
r/tests/testthat/test-dataset-dplyr.R:
##########
@@ -73,7 +73,7 @@ test_that("filter() on timestamp columns", {
   ds <- open_dataset(dataset_dir, partitioning = schema(part = uint8()))
   expect_equal(
     ds %>%
-      filter(ts >= lubridate::ymd_hms("2015-05-04 03:12:39")) %>%
+      filter(ts >= lubridate::ymd_hms("2015-05-04 03:12:39", tz = "UTC")) %>%

Review Comment:
   Is this change related to this PR?



##########
r/R/arrow-datum.R:
##########
@@ -103,10 +103,10 @@ Ops.ArrowDatum <- function(e1, e2) {
 #' @export
 Math.ArrowDatum <- function(x, ..., base = exp(1), digits = 0) {
   switch(.Generic,
-    abs = ,
-    sign = ,
-    floor = ,
-    ceiling = ,
+    abs = eval_array_expression("abs_checked", x),
+    sign = eval_array_expression("sign", x),
+    floor = eval_array_expression("floor", x),
+    ceiling = eval_array_expression("ceil", x),

Review Comment:
   This change seems unrelated to this PR. If it's not directly related to the `pkg::` thing, you should remove it.



##########
r/R/dplyr-funcs.R:
##########
@@ -116,3 +124,11 @@ create_binding_cache <- function() {
 nse_funcs <- new.env(parent = emptyenv())
 agg_funcs <- new.env(parent = emptyenv())
 .cache <- new.env(parent = emptyenv())
+
+register_bindings_utils <- function() {
+  register_binding("::", function(lhs, rhs) {
+    lhs_name <- as.character(substitute(lhs))
+    rhs_name <- as.character(substitute(rhs))
+    nse_funcs[[paste0(lhs_name, "::", rhs_name)]]

Review Comment:
   Here you should check if the function is actually in `nse_funcs`...if it isn't, you should return what base R would return which I think is `asNamespace(lhs_name)[[rhs_name]]`.
   
   (this is what's causing what you thought was an error in `filter()` but I think also affected `mutate()`)



##########
r/tests/testthat/test-dplyr-filter.R:
##########
@@ -198,7 +198,7 @@ test_that("Negative scalar values", {
 test_that("filter() with between()", {
   compare_dplyr_binding(
     .input %>%
-      filter(between(dbl, 1, 2)) %>%
+      filter(dplyr::between(dbl, 1, 2)) %>%

Review Comment:
   I wouldn't change this..."normal" usage is the test before the change...if you want to test namespacing you should do it in separate tests. Here and below you are testing two things at once: the behaviour of between and the behaviour of the namespacing. The namespacing is a separate concern and should be tested separately.



##########
r/tests/testthat/test-dplyr-funcs.R:
##########
@@ -19,21 +19,24 @@ test_that("register_binding() works", {
   fake_registry <- new.env(parent = emptyenv())
   fun1 <- function() NULL
 
-  expect_null(register_binding("some_fun", fun1, fake_registry))
+  expect_null(register_binding("some.pkg::some_fun", fun1, fake_registry))
   expect_identical(fake_registry$some_fun, fun1)
+  expect_identical(fake_registry$`some.pkg::some_fun`, fun1)
 
-  expect_identical(register_binding("some_fun", NULL, fake_registry), fun1)
+  expect_identical(register_binding("some.pkg::some_fun", NULL, fake_registry), fun1)
   expect_false("some_fun" %in% names(fake_registry))
-  expect_silent(expect_null(register_binding("some_fun", NULL, fake_registry)))
+  expect_silent(expect_null(register_binding("some.pkg::some_fun", NULL, fake_registry)))
 
-  expect_null(register_binding("some_pkg::some_fun", fun1, fake_registry))
+  expect_null(register_binding("somePkg::some_fun", fun1, fake_registry))
   expect_identical(fake_registry$some_fun, fun1)
 })
 
 test_that("register_binding_agg() works", {
   fake_registry <- new.env(parent = emptyenv())
   fun1 <- function() NULL
 
-  expect_null(register_binding_agg("some_fun", fun1, fake_registry))
+  expect_null(register_binding_agg("somePkg::some_fun", fun1, fake_registry))
+  names(fake_registry)

Review Comment:
   ```suggestion
     expect_null(register_binding_agg("somePkg::some_fun", fun1, fake_registry))
   ```



##########
r/R/dplyr-funcs.R:
##########
@@ -58,13 +58,20 @@ NULL
 #' @keywords internal
 #'
 register_binding <- function(fun_name, fun, registry = nse_funcs) {
-  name <- gsub("^.*?::", "", fun_name)
-  namespace <- gsub("::.*$", "", fun_name)
+  if (fun_name == "::") {

Review Comment:
   Maybe change `name` to `qualified_name` here to improve the readability?



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org