paleolimbot commented on code in PR #35398:
URL: https://github.com/apache/arrow/pull/35398#discussion_r1220725288
##########
r/R/filesystem.R:
##########
@@ -384,7 +387,24 @@ S3FileSystem <- R6Class("S3FileSystem",
region = function() fs___S3FileSystem__region(self)
)
)
-S3FileSystem$create <- function(anonymous = FALSE, ...) {
+S3FileSystem$create <- function(
+ anonymous = FALSE,
+ log_level = c("fatal", "error", "warn", "info", "debug", "trace", "off"),
+ ...) {
+ # Init S3 now only if log_level is set
+ # TODO: This is clunky
Review Comment:
Ah, I should have done a better job revisiting the discussion/reviewing the
first time!. I think putting it as an argument inside `S3Filesystem$create()`
is goofy because the lifecycle of the parameter has nothing to do with the
lifecycle of the filesystem that is returned. Somebody is much more likely to
attempt `s3_bucket(log_level = ...)` with varying log levels than put calls to
`s3_initialize()` all over the place.
Of course, any of these alternatives is better than none, which is what we
currently have! I don't really have a strong feeling here and am happy to defer
to earlier discussion.
To more literally help with the original comment here, you could do:
```r
$create <- function(log_level = NULL, ...) {
log_level <- log_level %||% "whater_the_default_value_is"
log_level <- rlang::arg_match(log_level, c("vebose", "other", "choices"))
...
}
```
...and list the choices in the documentation.
--
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]
