Thoughts on Error-Handling (request for comments) #264

Open
adamstallard opened this Issue Jan 23, 2013 · 9 comments

3 participants

@adamstallard

I wrote a wiki page that outlines the current state of error-handling in vows (discounting bugs, such as #263) and how it could be improved.

Thoughts on Error Handling

@adamstallard

I'm more-or-less asking for permission to implement this. I was trying to write some documentation about error-handling for the vowsjs site, and it struck me (once again) how hard to understand it is. Also, error-handling is currently broken even according to existing specifications. (Although fixed by #263)

@adamstallard

Here is my ongoing attempt at documentation:

Error Handling Documentation

@craigyk

Hi, I'm just a novice, but the current error handling confuses me... mostly because of how context specific it is. I've read through the Error Handling Documentation and Thoughts and it strikes me that it might just be better if all vows and subtopics are just passed all the callback arguments (including errors). I know it's more boilerplate, but you've already given some examples on how people can create more application specific interfaces on top of vows... maybe this should be similar?

Is one issue the pretty handling of errors in the vows output? since you already extend the assert module, maybe an assert.onError method could produce similar output? or maybe explicitly calling a callback.error method? IMO this would be more straighforward than setting behaviour modifying flags on the callback (callback.multi, etc.)

One particularly confusing aspect is that sub-topics also behave differently than vows. Why would one want to continue processing a sub-topic if an error is produced in a parent topic? I'm not sure but I'm sure one day I'll find a need... maybe to test error handling paths?

@adamstallard

Thanks for the comments, Craig.

the current error handling confuses me...

The current error handling confuses me, too--which is why I want to have this discussion.

and it strikes me that it might just be better if all vows and subtopics are just passed all the callback arguments (including errors)

This maybe the best; I think it is the best option we currently have. This is what happens if you set the "error" option to "false" on a suite--although this is undocumented. This is what I do in my own projects. Then at the top of each and every vow I have

assert.ifError(error);

Extra boilerplate, like you say, but it makes more sense to me than the default behavior (which I tried to document).

Currently, sub-topics get the arguments from the parent with the errors stripped out. That is o.k. with me (but it probably should be documented better); If I am testing the errors from a parent topic, I will pass them down to the sub-context using this.

or maybe explicitly calling a callback.error method?

The problem is that you don't always have control over how this.callback gets called. Different packages handle callbacks differently. If all packages always used the callback(error, data) paradigm, it would be easy. The behaviour-modifying flags are to signal to the context how the callback is going to get called. I think this does have to be set at the context-level since each context could be sending the callback to a different package.

you've already given some examples on how people can create more application specific interfaces on top of vows...

Letting vows treat every callback as callback(error, data) and wrapping the ones that are different is certainly an option, but I didn't like it--it seemed ugly and confusing.

One particularly confusing aspect is that sub-topics also behave differently than vows. Why would one want to continue processing a sub-topic if an error is produced in a parent topic? I'm not sure but I'm sure one day I'll find a need... maybe to test error handling paths?

That's the only reason I have found, but I would be o.k. with sub-topics not getting processed if the parent topic errors--I think you could probably always just process them in a vow at the parent level. Maybe someone else can think of another reason?

@craigyk

I agree with all your responses and am glad that you disable automatic error handling in the suite to always have the errors passed and handled by assert. Maybe eventually this will be the default? I have two additional arguments not stripping the error from sub-topics:

  1. It's conceptually cleaner, (less surprise) to users to not strip errors (first arguments), even with sub-topics. Also, what if someone is using a topic callback that returns multiple values, and the first one isn't an error?

  2. Removing all special handling of callback arguments (just forwarding them blindly) will probably simplify the code some?

@adamstallard

Also, what if someone is using a topic callback that returns multiple values, and the first one isn't an error?

Currently, the default is that you will get all the values returned to the vow, in arguments 2+. Argument 1 will be a placeholder for an error (the topic could error before it gets to the callback).*

I think this works fine if you are handling all your own errors (i.e. setting error=false on all suites). But this is surprising and confusing if your other callbacks are standard node callbacks in the form (error, data) and you are used to vows handling errors for you.

*The actual current behaviour (due to a bug to be fixed by #263) is that if your vow takes multiple arguments and the topic has an error in it, the first argument (the error placeholder) will be always null and argument 2 will contain the error.

@indexzero
vowsjs member

@adamstallard are you still interested in this? Sorry to leave you hanging for so long.

@indexzero indexzero referenced this issue Nov 4, 2014
Open

Bucket list of changes for v1.0.0 #318

0 of 4 tasks complete
@adamstallard

I am interested in this; I want us to come to a consensus on the syntax. #307 has this.callbackWithoutError as a separate function. I have this.callback.errors as a modifier to this.callback which you set (to false) before you call it.

@indexzero
vowsjs member

@adamstallard I agree. I am still parsing through all of this after being hands-off for so long. I've tried to tag all of the relevant issues as "error-handling".

Any thoughts @JerrySievert @davglass @samlecuyer?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment