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

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

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


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

Review Comment:
   I haven't thought about this in great detail, but I'd be expecting an implementation of this to use the `namespace` variable here, or remove it.
   
   I'm just wondering if there might be some advantages to recording the function and namespace separately instead of appending them?  The former feels "cleaner" in my head, though that's not sufficient reason to suggest changing it.
   
   A thought; how do we distinguish between different kinds of functions?  i.e. we have:
   
   * bindings for functions from packages, e.g. `lubridate::as_datetime()`
   * bindings directly to arrow compute functions, e.g. `arrow_utf8_split_whitespace()`
   * other bindings list the one for `cast()`
   
   In the last case here, if we have other functions which have underscores but no namespace, I think the regex would no longer be extracting the right name.



-- 
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