sprintf too few args

[tmpfs]

I believe this line:

https://github.com/davepacheco/node-verror/blob/master/lib/verror.js#L114

Should be > 1 not > 0.

I am using restify and if I have already created a message with util.format() that includes a URL that has % symbols in it as it has encoded portions then the call to sprintf at line 115 generates an error in the included sprintf library.

Error: too few args to sprintf

I can't see any reason to call sprintf when the argument list has a length of one.

Feel free to correct me though.

[davepacheco]

I think the suggested fix just papers over the bug without addressing the root cause. You're presumably doing something like this:

new WError('http://someserver/some%20encoded_url')

but it's just as wrong if you were to do something like:

new WError('http://%s/some%20encoded_url', 'someserver')

but the fix wouldn't address that. The right solution is to escape '%' signs before passing them to a sprintf-like function (including the WError constructor).

If you know you don't have any such formatters that you want to expand, you can use:

new WError('%s', your_string)

instead of

new WError(your_string)

[tmpfs]

Fair point, although I still think if the argument list is one that there are no replacement parameters and therefore no reason to invoke sprintf, just pass the message through untouched.

Clearly escaping % to %% is not really practical, but I am happy to do new WError('%s', your_string).

Out of interest, what is the reason to call sprintf if the argument list length is one?

[davepacheco]

For the same reason sprintf(3C) doesn't just call bcopy(3C) if there are no '%' signs in your format string? Because it's more code to check for that case, there's no compelling reason to handle it differently, and the resulting inconsistency makes it slightly more confusing? (I'd be surprised if I found out my code was passing something with a bare '%' to a sprintf-like function and it didn't crash.)

This case is more of a "meh", since the only change it can make is to make a program that would otherwise crash not crash, but that might be a bad change if that causes the program to produce incorrect output.

[tmpfs]

But surely you would agree with the statement that raising an error should never have the side effect of raising an error? Or not?

Consider this use case I have a program that generates a ValidationError which passes user input as a replacement parameter. I then have a preformatted message so I generate a RestError in restify (using the validation error message). If the user input contains a % I get an error while raising the error. This just seems plain wrong to me. It's a trivial fix to implement.

[davepacheco]

Sorry that this has been quiet for so long. I do disagree with your statement that:

raising an error should never have the side effect of raising an error

It depends on the kinds of the two errors. The process of raising an operational error can result in a programming error, in which case the program should immediately crash (as with all programming errors). As I just commented in #13: I agree that this is annoying, and I run into it too, but IMO it's the best way to stamp out programming errors, and production systems have to be built to survive these transient failures anyway. (What if you accessed an undefined property when constructing the error message?) See http://www.joyent.com/developers/node/design/errors for a more complete discussion on this.

[tmpfs]

Thanks for the feedback, closing as not an issue.