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

enhancement: handle search API error gracefully #80

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

hemal7735
Copy link
Collaborator

This PR addresses issue #78 - Handle Errors Better.

How was this tested?

  • Disconnect from network and run good-first-issue node, one should see the custom error message instead of Promise error

Copy link

@jimmiehansson jimmiehansson left a comment

Choose a reason for hiding this comment

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

Awesome work, a couple of questions

bin/good-first-issue.js Outdated Show resolved Hide resolved
bin/good-first-issue.js Outdated Show resolved Hide resolved
} catch (e) {
console.log('Oops! We encountered an issue while searching. Please check your network connection.')
process.exit(1)
}

if (issues.length === 0) {

Choose a reason for hiding this comment

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

If at this point we were to intercept an empty string literal from the response header rather than the results of a list, and this string literal was to be an error response in a short-circuit and no error was able to be parsed, edge of timeouts, parsing errors etc. This would still return as true, hence returning an empty string exposed.
It would be suitable at some point here validate the actual type integrity of the expected data formats being used, as side effects of this may lead to false positives.

@bnb bnb added the enhancement New feature or request label Dec 16, 2018
@bnb
Copy link
Member

bnb commented Jul 25, 2019

@hemal7735 would you like to try to modernize this PR? Happy address if it's still relevant with the changes to master.

@hemal7735
Copy link
Collaborator Author

@hemal7735 would you like to try to modernize this PR? Happy address if it's still relevant with the changes to master.

Hello @bnb, thanks for waiting. I have brought the PR upto speed with the master branch. Please review when you get the time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants