thisisnic commented on code in PR #35398:
URL: https://github.com/apache/arrow/pull/35398#discussion_r1183315456
##########
r/R/filesystem.R:
##########
@@ -442,6 +442,57 @@ default_s3_options <- list(
request_timeout = -1
)
+# Enumerate possible values for AWS SDK log levels, used by s3_init
+s3_log_levels <- c("off", "fatal", "error", "warn", "info", "debug", "trace")
Review Comment:
Instead of defining these here, you can set the `log_level` parameter in the
function definition to this vector instead.
##########
r/R/filesystem.R:
##########
@@ -442,6 +442,57 @@ default_s3_options <- list(
request_timeout = -1
)
+# Enumerate possible values for AWS SDK log levels, used by s3_init
+s3_log_levels <- c("off", "fatal", "error", "warn", "info", "debug", "trace")
+
+#' Initialize S3
+#'
+#' Must be called before other S3 functions such as \link{s3_bucket}.
+#'
+#' @param log_level string for log level to set. See details.
+#' @param num_event_loop_threads How many threads to use for the AWS SDK's I/O
+#' event loop. Defaults to 1.
+#' @details The parameter `log_level` must be one of: off, fatal, error, warn,
+#' info, debug, or trace and is case insensitive.
+#'
+#' @examples
+#' # Initialize S3 with Debug-level logging
+#' \dontrun{
+#' s3_init("debug")
+#' }
+s3_init <- function(log_level, num_event_loop_threads = 1) {
+ stopifnot(is.character(log_level))
+ stopifnot(is.numeric(num_event_loop_threads))
+
+ log_level_normalized <- tolower(log_level)
+
+ # Validate log_level
+ if (!(log_level_normalized %in% s3_log_levels)) {
+ stop(
+ "Argument 'log_level' must be one of: ",
+ paste(
+ paste(s3_log_levels[seq_len(length(s3_log_levels) - 1)], collapse = ",
"),
+ s3_log_levels[length(s3_log_levels)],
+ sep = ", or "),
+ paste0(" but was '", log_level_normalized, "'."),
+ call. = FALSE)
+ return(invisible())
+ }
+
+ # Validate num_event_loop_threads
+ if (num_event_loop_threads < 1) {
+ stop(
+ "Argument 'num_event_loop_threads' should be a positive integer ",
+ "greater than or equal to 1 but was '", num_event_loop_threads, "'.",
+ call. = FALSE
+ )
+
+ return(invisible())
Review Comment:
Why return `invisible()` here?
##########
r/R/filesystem.R:
##########
@@ -442,6 +442,57 @@ default_s3_options <- list(
request_timeout = -1
)
+# Enumerate possible values for AWS SDK log levels, used by s3_init
+s3_log_levels <- c("off", "fatal", "error", "warn", "info", "debug", "trace")
+
+#' Initialize S3
+#'
+#' Must be called before other S3 functions such as \link{s3_bucket}.
+#'
+#' @param log_level string for log level to set. See details.
+#' @param num_event_loop_threads How many threads to use for the AWS SDK's I/O
+#' event loop. Defaults to 1.
+#' @details The parameter `log_level` must be one of: off, fatal, error, warn,
+#' info, debug, or trace and is case insensitive.
+#'
+#' @examples
+#' # Initialize S3 with Debug-level logging
+#' \dontrun{
+#' s3_init("debug")
+#' }
+s3_init <- function(log_level, num_event_loop_threads = 1) {
+ stopifnot(is.character(log_level))
+ stopifnot(is.numeric(num_event_loop_threads))
+
+ log_level_normalized <- tolower(log_level)
+
+ # Validate log_level
+ if (!(log_level_normalized %in% s3_log_levels)) {
+ stop(
+ "Argument 'log_level' must be one of: ",
+ paste(
+ paste(s3_log_levels[seq_len(length(s3_log_levels) - 1)], collapse = ",
"),
+ s3_log_levels[length(s3_log_levels)],
+ sep = ", or "),
+ paste0(" but was '", log_level_normalized, "'."),
+ call. = FALSE)
+ return(invisible())
+ }
Review Comment:
This level of error messaging is awesome, and I love the detail here.
However, you can simplify the code in this function by setting the default
value of `log_level` to the vector of possible values and then using
`rlang::arg_match()` (see docs at
https://rlang.r-lib.org/reference/arg_match.html) to do the hard work for you.
I see you've accounted for converting values to lowercase, but this could be
handled elsewhere.
--
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]
