thisisnic commented on PR #35398: URL: https://github.com/apache/arrow/pull/35398#issuecomment-1534113610
It would make it easier for the user to run into this scenario: > > ```r > options(arrow.s3_log_level = "fatal") > s3_bucket() # Indirectly causes initialization of the API with log level set to Fatal > options(arrow.s3_log_level = "debug") # <---- Has no effect, log level stays set to Fatal > ``` > With the implementation in this PR, the user would instead get an error: > > ```r > > arrow:::s3_init(log_level = "debug") > Error: Invalid: S3 was already initialized. It is safe to use but the options passed in this call have been ignored. > ``` Ah, that's a shame; I agree that the silent swallowing of errors isn't great. Would it be any better if we called `s3_init()` inside `s3_bucket()` instead, and rather than using the `options` above, we update the signature of `s3_bucket()` to take a new parameter, `log_level` to define the value, or would we still run into the same problem? > I left it unexported just until it was discussed in review and since I wasn't sure. I'm leaning towards export but am curious what you think. I feel like it'd be helpful to expose this functionality to (advanced) end users, but it's still unclear how best to do this in a way which would be consistent with their expectations for how this would work. It's tricky! -- 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]
