[GitHub] [arrow] paleolimbot commented on a diff in pull request #34524: GH-34421: [R] Let GcsFileSystem take a path for json_credentials

["paleolimbot (via GitHub)" <gi...@apache.org>]

paleolimbot commented on code in PR #34524:
URL: https://github.com/apache/arrow/pull/34524#discussion_r1131865760


##########
r/R/filesystem.R:
##########
@@ -572,6 +572,11 @@ GcsFileSystem$create <- function(anonymous = FALSE, retry_limit_seconds = 15, ..
 
   options$retry_limit_seconds <- retry_limit_seconds
 
+  # Handle reading json_credentials from the filesystem
+  if ("json_credentials" %in% names(options) && file.exists(options[["json_credentials"]])) {
+    options[["json_credentials"]] <- paste(readLines(options[["json_credentials"]]), collapse = "")

Review Comment:
   I forget the exact details but I think `readLines()` assumes system encoding (rather than UTF-8, which matters on Windows). Any thought as to which one of those is a better assumption if the value points to a file? (The workaround would be for the user to read in the file themselves first with an explicit encoding).



##########
r/R/filesystem.R:
##########
@@ -572,6 +572,11 @@ GcsFileSystem$create <- function(anonymous = FALSE, retry_limit_seconds = 15, ..
 
   options$retry_limit_seconds <- retry_limit_seconds
 
+  # Handle reading json_credentials from the filesystem
+  if ("json_credentials" %in% names(options) && file.exists(options[["json_credentials"]])) {
+    options[["json_credentials"]] <- paste(readLines(options[["json_credentials"]]), collapse = "")

Review Comment:
   Also, did you want to `collapse = "\n"`?



##########
r/tests/testthat/test-gcs.R:
##########
@@ -91,6 +91,20 @@ test_that("GcsFileSystem$create() input validation", {
   )
 })
 
+test_that("GcsFileSystem$create() can read json_credentials", {
+  # From string
+  fs <- GcsFileSystem$create(json_credentials = "fromstring")
+  expect_equal(fs$options$json_credentials, "fromstring")
+
+  # From disk
+  cred_path <- tempfile()
+  on.exit(unlink(cred_path))
+
+  writeLines("fromdisk", cred_path)

Review Comment:
   Maybe put some non-ASCII text and here? In test-csv there should be an example of how to do this (there are some `\u` and `charToRaw()` and `writeBin` things and I always forget the details).



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org