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: surface the message when the check api returns an error #85

Merged
merged 2 commits into from
Feb 23, 2018

Conversation

davidmenendez
Copy link
Contributor

@davidmenendez davidmenendez commented Feb 20, 2018

What:
If the contributors API returns a bad response show the error message

Why:
I ran into this when I was running the check command. The CLI was throwing an error on this line https://github.com/jfmengels/all-contributors-cli/blob/master/src/util/check.js#L28 the response from the API was a 403 and I was getting rate limited. The response was something like

{ "message": "API rate limit exceeded" }

but there's no prior check to see if either the response is a 200 or if the body is an array. I think it makes sense to throw an error and tell the user what's happening.

How:
Updated the code to throw an error if the API response isn't 200.

Checklist:

  • Documentation
  • Tests
  • Ready to be merged
  • Added myself to contributors table

@davidmenendez davidmenendez changed the title surface the message when the api returns an error surface the message when the check api returns an error Feb 20, 2018
@codecov-io
Copy link

codecov-io commented Feb 20, 2018

Codecov Report

Merging #85 into master will decrease coverage by 0.01%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #85      +/-   ##
==========================================
- Coverage   52.81%   52.79%   -0.02%     
==========================================
  Files          18       18              
  Lines         320      322       +2     
  Branches       43       44       +1     
==========================================
+ Hits          169      170       +1     
- Misses        134      135       +1     
  Partials       17       17
Impacted Files Coverage Δ
src/util/check.js 95.23% <66.66%> (-4.77%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 687194c...352cb21. Read the comment docs.

Copy link
Collaborator

@machour machour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @davidmenendez and thank you for this proposal.
Left you a small feedback to address!

@@ -25,6 +25,9 @@ function getContributorsPage(url) {
})
.then(res => {
const body = JSON.parse(res.body)
if (res.statusCode !== 200) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

30x are also valid API response code (https://developer.github.com/v3/#http-redirects)

@davidmenendez
Copy link
Contributor Author

good point!

@machour machour changed the title surface the message when the check api returns an error fix: surface the message when the check api returns an error Feb 23, 2018
@machour machour merged commit ab41caa into all-contributors:master Feb 23, 2018
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

3 participants