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

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

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



##########
File path: r/tests/testthat/test-csv.R
##########
@@ -317,7 +317,7 @@ test_that("Write a CSV file with different batch sizes", {
 test_that("Write a CSV file with invalid input type", {
   expect_error(
     write_csv_arrow(Array$create(1:5), csv_file),
-    regexp = 'x must be a "ArrowTabular"'
+    regexp = "Cannot write to CSV. 'x' must be an object of class 'data.frame', 'RecordBatch', or 'Table', not 'Array'"

Review comment:
       This is also not strictly about this pull request, but I believe the 📦 would benefit from switching to testthat 3rd edition `expect_snaphot()` ... https://testthat.r-lib.org/articles/third-edition.html

##########
File path: r/R/util.R
##########
@@ -110,3 +110,9 @@ handle_embedded_nul_error <- function(e) {
   }
   stop(e)
 }
+
+check_tabular <- function(x, format){
+  if(!inherits(x, "ArrowTabular")){
+    stop(paste0("Cannot write to ", format, ". 'x' must be an object of class 'data.frame', 'RecordBatch', or 'Table', not '", class(x)[1], "'"))

Review comment:
       This might be more of a general thing than related to this specific pull request, but perhaps we could use `rlang::abort()` and take advantage of the bullets, e.g. 
   
   ```r
   abort(c(
     paste0("Cannot write to ", format, ".), 
     x = paste0("'x' must be an object of class 'data.frame', 'RecordBatch', or 'Table', not '", class(x)[1], "'")
   ))
   ```
   
   Or if we were willing to use `glue::` 
   
   ```r
   abort(c(
     glue("Cannot write to { format }."), 
     x = glue("'x' must be an object of class 'data.frame', 'RecordBatch', or 'Table', not '{ class(x)[1] }'")
   ))
   ```
   
   Relatedly, we tend to use backticks when naming objects and calls, i.e. 
   
   ```r
   glue("`x` must be an object") ...
   ```




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