Skip to content
This repository was archived by the owner on Mar 3, 2020. It is now read-only.

Added check on content-type in http error handling#56

Closed
simlay wants to merge 1 commit intocoinbase:masterfrom
simlay:uncaught_err
Closed

Added check on content-type in http error handling#56
simlay wants to merge 1 commit intocoinbase:masterfrom
simlay:uncaught_err

Conversation

@simlay
Copy link

@simlay simlay commented Mar 2, 2016

This is for #55

I'll be honest, my Node isn't perfect I'm all for a cleaner solution if suggested.

@jorilallo
Copy link
Contributor

Thanks, this looks great. I'll need to test this as we're lacking test coverage for these tests cases so will merge later

@simlay
Copy link
Author

simlay commented Mar 9, 2016

I'm all for writing some tests.

My only issue is that the nock http implementation doesn't have the getHeaders on a response object. Though, I could be using it wrong.

@jorilallo Do you have any suggestions on this?

{name: _convertErrorName(errorBody.id)});
} else {
error = createError(response.statusCode,
'InternalServerError');
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer still keeping the actual error message here. Return response.body rather than masking it with 'InternalServerError'

Alternatively,

var errorBody;
try { 
    errorBody = _parseError(JSON.parse(response.body));
 } catch (ex) {
   errorBody = { id: -1, message: response.body };
}
...

@CjS77 CjS77 mentioned this pull request Apr 25, 2016
@jorilallo
Copy link
Contributor

@simlay thanks for raising this. @CjS77 actually pushed a fix for this so I'll close this one. Sorry for the long turnaround time

@jorilallo jorilallo closed this Apr 25, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants