Handle array of errors. - #289
Conversation
|
@bajtos, the test case here is a little weird, please advise. |
| data.stack = err.stack; | ||
| if (process.env.NODE_ENV === 'production' || options.disableStackTrace) { | ||
| delete data.stack; | ||
| debug('Error in %s %s: %s', req.method, req.url, err.stack); |
There was a problem hiding this comment.
This is redundant, because generateResponseError already prints a debug message. Please remove.
There was a problem hiding this comment.
On the second thought, I think it may be a bit surprising that generateResponseError prints a debug log as a side-effect. Even you, as an author, got confused.
I am proposing to remove the debug statement from generateResponseError, and add explicit debug() calls in all places where it makes sense. Please check the resulting debug output for both cases (single error, array of errors) to ensure the information printed is useful and easy to understand.
There was a problem hiding this comment.
@bajtos
I didn't get confused, I just forgot to delete the line outside of generateResponseError. I think that the debug log should remain in generateResponseError as is. It will display the individual errors from the connector as well as the location in which they were aggregated.
It looks good to me. What do you find weird? It's possible I am missing something. |
|
@bajtos |
c496111 to
216a8f3
Compare
| var testError = new Error('expected test error'); | ||
| var errArray = [testError, testError]; | ||
|
|
||
| var expectedMessage = |
There was a problem hiding this comment.
I think this variable is no longer used, could you please remove?
I suppose you could simply check that the second request returns an error where Anyways, the current test looks good to me. |
|
@richardpringle I found few nitpicks, see the comments above. The patch LGTM otherwise, no further review is necessary. Please back-port to 2.x too. |
216a8f3 to
3783080
Compare
|
@bajtos, changes made but I don't quite know what's going on with these failed tests... Otherwise this is ready to land. |
|
@slnode please test |
|
@bajtos, there are random dependency tests failing. I haven't looked at all of them but in connector-rest there are 3 tests failing only on node 0.10 on ubuntu and in lb-boot, linter is failing for node 5 on ubuntu and amazon. Linter is also failing on all platforms for lb-workspace. I'll check to see if the master branch manages to pass CI but in the meantime, do you have any ideas? |
|
This build hasn't passed in 6 months... |
| }); | ||
|
|
||
| err = | ||
| new Error('Failed with multiple errors, see `details` for more information.'); |
There was a problem hiding this comment.
Nitpick: I personally prefer this style:
var err = new Error(
'Failed with multiple errors, see `details` for more information.');or even the following, which works better when the error message needs to be split onto multiple lines:
var msg = 'Failed with multiple errors, see `details` for more information.';
var err = new Error(msg);Feel free to ignore this comment.
There was a problem hiding this comment.
@bajtos, I will not ignore this comment! Haha, I completely agree. While I'm not a huge fan of your first suggestion, I do prefer your second. I usually do that, not sure why I didn't this time.
Regardless, I fixed it
|
LGTM. As for the failing CI builds of dependencies, I hate to say it, but we should ignore those random failures and simply land this PR. |
3783080 to
8d439c9
Compare
2 participants
Temporary fix for strongloop/loopback#2123
Connect to strongloop/loopback#2123