[GitHub] [arrow] nealrichardson commented on a change in pull request #10387: ARROW-12761: [R] Better error handling for write_to_raw

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

nealrichardson commented on a change in pull request #10387:
URL: https://github.com/apache/arrow/pull/10387#discussion_r643343363



##########
File path: r/R/util.R
##########
@@ -110,3 +110,19 @@ handle_embedded_nul_error <- function(e) {
   }
   stop(e)
 }
+
+check_tabular <- function(x, format) {

Review comment:
       I like factoring out a function to encapsulate common logic, but I have some concerns with this one. The name is too generic--I would expect `check_tabular <- function(x) inherits(x, "ArrowTabular")`--but this is much more specific and doesn't really describe what it does, which I think will make this harder to maintain and use going forward.
   
   * Nothing about the function name suggests that it is only for writing to a format, but the error message says that
   * It checks whether you have `ArrowTabular` but then the error message says data.frame or ArrowTabular. This happens to work because you call it after converting data.frame to Table inside each function where this is used, but it wouldn't necessarily work if you used it in another function.
   * It says "`x` must be ..." but nothing guarantees that the argument name in the parent function is called `x`
   
   Some suggestions:
   
   * Rename to `assert_` something since, like the other assert_ functions, this raises an error if the condition is not met. 
   * Either make the full error message (i.e. what currently is `paste0("Cannot write to ", format, ".")` a function argument so that the function isn't specific to writing, or put "write" somewhere in the function name so that it's clear that it's only for validating writing.
   * Use some `call` parsing to get the actual argument name from the caller (that is, don't assume it's called `x`) (Would `asserthat` do this for you, or have code you can refer to for inspiration?)
   * Change the assertion to `if (!inherits(x, c("data.frame", "ArrowTabular")))` so that it matches the message. 




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

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