-
Notifications
You must be signed in to change notification settings - Fork 123
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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
} catch (e) { | ||
console.log('Oops! We encountered an issue while searching. Please check your network connection.') | ||
process.exit(1) | ||
} | ||
|
||
if (issues.length === 0) { |
There was a problem hiding this comment.
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.
@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. |
This PR addresses issue #78 - Handle Errors Better.
How was this tested?
good-first-issue node
, one should see the custom error message instead ofPromise
error