jonkeane commented on a change in pull request #12170:
URL: https://github.com/apache/arrow/pull/12170#discussion_r788194412
##########
File path: r/R/dataset-format.R
##########
@@ -309,14 +309,52 @@ FileWriteOptions <- R6Class("FileWriteOptions",
inherit = ArrowObject,
public = list(
update = function(table, ...) {
+ compose_err_header <- function(unsupported_passed_args) {
+ paste0(
+ oxford_paste(unsupported_passed_args),
+ ngettext(length(unsupported_passed_args),
+ " is not a valid argument ",
+ " are not valid arguments "),
+ "for your chosen `format`."
+ )
+ }
Review comment:
Nice, pulling this out into a separate function like this. I wonder if
we could expand this even more to include the actual `abort()` call, and the
following:
```
unsupported_passed_args <- setdiff(names(args), supported_args)
if (length(unsupported_passed_args)) {
err_header <- compose_err_header(unsupported_passed_args)
abort(err_header)
}
```
Basically, something like:
```
compose_err_header <- function(passed_args, format) {
...
if (length(...) > 0) {
abort(...)
}
}
```
or:
```
compose_err_header <- function(passed_args, supported_args) {
...
if (length(...) > 0) {
abort(...)
}
}
```
##########
File path: r/R/dataset-format.R
##########
@@ -309,14 +309,52 @@ FileWriteOptions <- R6Class("FileWriteOptions",
inherit = ArrowObject,
public = list(
update = function(table, ...) {
+ compose_err_header <- function(unsupported_passed_args) {
+ paste0(
+ oxford_paste(unsupported_passed_args),
+ ngettext(length(unsupported_passed_args),
+ " is not a valid argument ",
+ " are not valid arguments "),
+ "for your chosen `format`."
Review comment:
could we? should we? also offer what the supported arguments are (if we
have them as a list already...)
##########
File path: r/R/dataset-format.R
##########
@@ -309,14 +309,52 @@ FileWriteOptions <- R6Class("FileWriteOptions",
inherit = ArrowObject,
public = list(
update = function(table, ...) {
+ compose_err_header <- function(unsupported_passed_args) {
+ paste0(
+ oxford_paste(unsupported_passed_args),
+ ngettext(length(unsupported_passed_args),
+ " is not a valid argument ",
+ " are not valid arguments "),
+ "for your chosen `format`."
+ )
+ }
+
if (self$type == "parquet") {
+ args <- list(...)
+ supported_args <- names(formals(write_parquet))
+ supported_args <- supported_args[supported_args != c("x", "sink")]
+ unsupported_passed_args <- setdiff(names(args), supported_args)
+
+ if (length(unsupported_passed_args)) {
+ err_header <- compose_err_header(unsupported_passed_args)
+ abort(err_header)
+ }
+
dataset___ParquetFileWriteOptions__update(
self,
ParquetWriterProperties$create(table, ...),
ParquetArrowWriterProperties$create(...)
)
} else if (self$type == "ipc") {
args <- list(...)
+ supported_args <- c(
+ "use_legacy_format",
+ "metadata_version",
+ "codec",
+ "null_fallback"
+ )
+
+ unsupported_passed_args <- setdiff(names(args), supported_args)
+ err_info <- NULL
+
+ if (length(unsupported_passed_args)) {
+ err_header <- compose_err_header(unsupported_passed_args)
+ if("compression" %in% unsupported_passed_args) {
Review comment:
```suggestion
if ("compression" %in% unsupported_passed_args) {
```
Though this might be refactored if you take the suggestions up above
--
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]
