paleolimbot commented on code in PR #13160:
URL: https://github.com/apache/arrow/pull/13160#discussion_r921735078
##########
r/tests/testthat/test-dplyr-funcs-datetime.R:
##########
@@ -201,6 +212,14 @@ test_that("strftime", {
times
)
+ # with namespacing
Review Comment:
That pattern is probably a result of an earlier review from me - I think it
would be sufficient to test the mechanism for `::` in each verb without
checking that the namespaced version of every function works. I wouldn't have
expected the duplicated tests unless there were functions that were
special-cased (when reading the diff it made me wonder if some functions *were*
special-cased).
##########
r/R/dplyr-summarize.R:
##########
@@ -161,6 +161,21 @@ register_bindings_aggregate <- function() {
})
}
+# we register 2 version of the "::" binding - one for use with nse_funcs
+# and another one for use with agg_funcs (below)
Review Comment:
Maybe?
```suggestion
# we register 2 version of the "::" binding - one for use with agg_funcs
# and another one for use with nse_funcs (above)
```
##########
r/R/dplyr-funcs.R:
##########
@@ -58,15 +58,34 @@ NULL
#' @keywords internal
#'
register_binding <- function(fun_name, fun, registry = nse_funcs) {
- name <- gsub("^.*?::", "", fun_name)
- namespace <- gsub("::.*$", "", fun_name)
+ qualified_name <- fun_name
+ if (qualified_name == "::") {
+ unqualified_name <- "::"
+ } else {
+ unqualified_name <- gsub("^.*?::", "", qualified_name)
+ }
- previous_fun <- if (name %in% names(registry)) registry[[name]] else NULL
+ previous_fun <- if (unqualified_name %in% names(registry))
registry[[unqualified_name]] else NULL
+
+ # if th unqualified name exists in the register, warn
+ if (!is.null(fun) && !is.null(previous_fun)) {
+ warn(
+ paste0(
+ "A \"",
+ unqualified_name,
+ "\" binding already exists in the register and will be overwritten.")
+ )
+ }
+ # if fun is NULL remove entries from the function registry
Review Comment:
I am almost certainly who added the NULL-means-remove thing...the ability to
remove a binding is only relevant if or when we export this function (so if you
haven't already and/or it makes something in this PR difficult, feel free to
remove it!)
--
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]
