search

ropensci / piggyback

:package: for using large(r) data files on GitHub
https://docs.ropensci.org/piggyback
GNU General Public License v3.0
185 stars 26 forks source link

handle empty tokens from gh::gh_token, resolves #77 #78

Closed tanho63 closed 2 years ago

tanho63 commented 2 years ago

I identified the problem in #76 and #77 as related to https://github.com/ropensci/piggyback/blob/master/R/pb_download.R#L126 - gh::gh_token() returns "" if no environment variable or token is found, which creates an add_headers(Authorization = paste("token", "") call, causing a bad credentials/401 error.

cboettig commented 2 years ago

@tanho63 I'm open to more discussion as to whether we stop_for_status or warn_for_status. The former could be more of a nuisance in large batch runs, forcing users to work around with tryCatch or equivalent, but may indeed be the most reasonable behavior by default.

tanho63 commented 2 years ago

I had initially thought stop_for_status() was better, but I re-read the contents of the prev issue and apparently it still returns the correct content sometimes despite the 401 error? (This seems doubtful, I wonder if it wasn't being checked properly...)

I think it's misleading when something gets written to the destination file that isn't the correct contents, but if it sometimes works I guess I don't want to break it? That said if I was writing this from scratch I'd definitely use stop instead of warn. Happy to defer to whatever you think best