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

Check documentation url suffix #17

Merged
merged 4 commits into from
Oct 25, 2023
Merged

Conversation

bntzio
Copy link
Contributor

@bntzio bntzio commented Oct 20, 2023

Apparently GitHub changed the documentation_url value, thus not detecting the rate limit.

Here's the response I'm getting when hitting a secondary rate limit:

{
  "documentation_url": "https://docs.github.com/en/free-pro-team@latest/rest/overview/resources-in-the-rest-api#secondary-rate-limits",
  "message": "You have exceeded a secondary rate limit. Please wait a few minutes before you try again. If you reach out to GitHub Support for help, please include the request ID [REQUEST_ID]."
}

Instead of comparing the exact same documentation url, we can instead check for the url suffix, which has been the same across changes, this to fix and future-proof any existing url changes in the future as well.

This is similar to the case of the message, when we check for the prefix.

@gofri
Copy link
Owner

gofri commented Oct 24, 2023

Hi @bntzio, thank you very much for your contribution! This is highly appreciated!
Unfortunately I'm not available to handle this PR right now because of the situation in Israel; I'm on reserved duty. I hope I'll find some time this week to merge it and create a new release.

Meanwhile, it would be great if you can add any or all of the following:

  • A reference to the behavior change in GitHub's API, if there's any.
  • A test that checks for both the old and the new response, to avoid breaking anything.
  • A comment in the code that describes the logic behind the new check.

Thanks!

@bntzio
Copy link
Contributor Author

bntzio commented Oct 25, 2023

Hi @gofri, no problem! Happy to contribute!

No worries, take your time, I hope everything is good.

I just added all the points you just mentioned, regarding the reference for the GitHub API change, I couldn't find any change on their docs, I guess is not a change across all GitHub users, but maybe it is based at the org membership level, anyway, this change should support all the cases since it's checking for the path suffix, which is the same across all the GitHub API versions.

Thanks for making this, it is really useful!

Take care!

@gofri gofri merged commit 8b30f0d into gofri:main Oct 25, 2023
2 checks passed
@gofri
Copy link
Owner

gofri commented Oct 25, 2023

@bntzio thanks! merged and tagged :)

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.

3 participants