paleolimbot commented on a change in pull request #12319:
URL: https://github.com/apache/arrow/pull/12319#discussion_r800684130



##########
File path: r/R/dplyr-funcs-type.R
##########
@@ -248,3 +249,33 @@ register_bindings_type_elementwise <- function() {
     is_inf & !call_binding("is.na", is_inf)
   })
 }
+
+register_bindings_type_format <- function() {
+  register_binding("format", function(x, ...) {
+    if (inherits(x, "Expression") &&
+        x$type_id() %in% Type[c("TIMESTAMP", "DATE32", "DATE64")]) {
+      binding_format_datetime(x, ...)
+    } else {
+      abort(paste0("`format()` not yet supported for `", class(x$type())[[1]], 
"`"))
+    }
+  })
+}
+
+binding_format_datetime <- function(x, format = "", tz = "", usetz = FALSE) {

Review comment:
       I would personally put this in dplyr-funcs-datetime.R since it's closely 
related to strftime and friends (but I don't feel strongly about this).

##########
File path: r/R/dplyr-funcs-type.R
##########
@@ -248,3 +249,33 @@ register_bindings_type_elementwise <- function() {
     is_inf & !call_binding("is.na", is_inf)
   })
 }
+
+register_bindings_type_format <- function() {
+  register_binding("format", function(x, ...) {
+    if (inherits(x, "Expression") &&

Review comment:
       I think here you need to handle the case where `x` is *not* an 
expression (e.g., if a user calls `format(1234.393483)`). Perhaps an `if 
(!inherits(x, "Expression)) { return (format(x, ...)) }` at the top?

##########
File path: r/R/dplyr-funcs-type.R
##########
@@ -248,3 +249,33 @@ register_bindings_type_elementwise <- function() {
     is_inf & !call_binding("is.na", is_inf)
   })
 }
+
+register_bindings_type_format <- function() {
+  register_binding("format", function(x, ...) {
+    if (inherits(x, "Expression") &&
+        x$type_id() %in% Type[c("TIMESTAMP", "DATE32", "DATE64")]) {
+      binding_format_datetime(x, ...)
+    } else {
+      abort(paste0("`format()` not yet supported for `", class(x$type())[[1]], 
"`"))

Review comment:
       I think that `x$cast(string())` or similar would be a better default 
that would handle a lot of common cases.




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


Reply via email to