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

[WIP] Add status codes to error objects #16

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

Conversation

Nmargolis
Copy link

@Nmargolis Nmargolis commented Aug 28, 2018

This is work in progress to add status codes to error objects, as proposed in #15.

Next steps

  • Would love a review of the overall approach
  • There are many other errors returned for things like validation of inputs. Should those also have a status property on them? I'm not sure what it would be, if so, because those aren't related to http responses

cc @ingalls

Copy link
Contributor

@ingalls ingalls left a comment

Choose a reason for hiding this comment

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

👍 This looks like a good first step!

there is a lot of duplicate code here to copy the status code into the error object, what would you think about making this a generic function.

IE a new function called HTTPError

looking something like:

HTTPError(text, res) -> returns err with status

then these could go back to being oneliners like

return callback(HTTPError('I AM AN ERROR', res));

lib/auth.js Outdated
if (res.statusCode !== 200) return cb(new Error(res.body));
if (res.statusCode === 404) {
const error = new Error('404: Could not obtain auth list');
error.status = res.statusCode;
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to return the error here.

@Nmargolis
Copy link
Author

@ingalls great idea to make an HTTPError class! There is now an errors module in case we want to create other error classes in the future. Happy to make HTTPError it's own module if you think that's not a realistic possibility.

Also not sure if we should throw an error if the response parameter is missing, or if we should just make the status code null if so (which is what I've done, since it seems strange to throw an error when creating an Error instance 😂). Happy to change that too.

@ingalls
Copy link
Contributor

ingalls commented Aug 29, 2018

This is fantastic! Looks good to go to me! - Thanks a ton for adding tests

@ingalls ingalls mentioned this pull request Sep 23, 2019
4 tasks
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.

2 participants