Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix github_error() for when API does not return ratelimit #413

Merged
merged 4 commits into from Feb 13, 2020

Conversation

dpprdan
Copy link
Contributor

@dpprdan dpprdan commented Jul 31, 2019

A Github (Enterprise) instance does not always have rate-limiting activated and the corresponding API therefore does not return a x-ratelimit-reset, see #396.

This fixes github_error() for when this is the case.

Without this fix, the error message for the query in #396 is:

Error: Failed to install 'unknown package' from GitHub:
  attempt to set an attribute on NULL

Now it is:

Error: Failed to install 'unknown package' from GitHub:
  HTTP error 401.
  Bad credentials

(The package is unknown, because github_error() is triggered by github_DESCRIPTION().)

I guess one could also add some guidance for 401 errors? Let me know if you want me too.

I also wondered whether the ratelimit information is indeed useful for non-429 HTTP errors? What do I do with that information, when it isn't causing the problem? If that info can be omitted, that could make the code cleaner, namely these three or four cases:

  1. 404 error
  2. possibly 401 error
  3. 429 error (+ ratelimit info)
  4. all other errors.

Please let me know what you think.

@jimhester
Copy link
Member

jimhester commented Aug 2, 2019

How about

res_headers$`x-ratelimit-reset` %||% NA

This way you avoid needing any additional conditionals format(.POSIXct(NA), usetz = TRUE) is NA, so it should be fine.

Seeing the rate limit information is useful even if it is not a 429 error, it lets me know if someone is using authenticated requests or not, (the limit is only 60 for unauthenticated requests).

@dpprdan
Copy link
Contributor Author

dpprdan commented Aug 4, 2019

%||% is better indeed.

I still left the last conditional in there, because IMHO the rate limit information in

Error: Failed to install 'MARMoSET' from GitHub:
  HTTP error 401.
  Bad credentials

  Rate limit remaining: NA/NA
  Rate limit reset at: NA

is potentially misleading from a user perspective (i.e. suggesting that something is wrong with the rate limit while the problem is the credentials). So right now the two rate limit lines won't be displayed (see my original post). But please let me know, if you want me to change that.

@jimhester
Copy link
Member

jimhester commented Aug 5, 2019

Cool, looks much cleaner now, thanks!
Can you please add a bullet to NEWS? It should briefly describe the change and end with (@yourname, #issuenumber), then I can merge this PR.

@dpprdan
Copy link
Contributor Author

dpprdan commented Aug 6, 2019

👍 done!

@jimhester jimhester merged commit f75e4be into r-lib:master Feb 13, 2020
0 of 2 checks passed
@jimhester
Copy link
Member

jimhester commented Feb 13, 2020

Thanks!

@dpprdan dpprdan deleted the fix/github_error_no_ratelimit branch Feb 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants