tdegrunt / jsonschema

JSON Schema validation
Other
1.83k stars 262 forks source link

Introduce ValidatorResultError that extends Error #264

Closed landau closed 4 years ago

landau commented 6 years ago

This causes problems when using ava for testing errors. ava is of the opinion that only values of type Error should be thrown. Specifically, when using t.throws.

Is there a reason that it doesn't "extend" Error?

Line: https://github.com/tdegrunt/jsonschema/blob/master/lib/helpers.js#L5

Ava test for asserting it must be an error. https://github.com/avajs/ava/blob/master/test/assert.js#L717-L728

awwright commented 6 years ago

@landau I don't think I considered the possibility that we'd want to throw a ValidationError, it's primary purpose is to contain information about the assertion failure, not be an error as such. Throwing errors when there's a validation problem is a poorly supported feature.

But, I don't see why we can't do that.

landau commented 6 years ago

Throwing an error on validation is a feature of this lib. 😄

Line: https://github.com/tdegrunt/jsonschema/blob/038fc524548ae19fd6fa5fb88bb08b9fea087ab6/lib/helpers.js#L51-L53

validate('foo', { type: 'number' }, { throwError: true });
awwright commented 6 years ago

@landau Right, but it's poorly implemented.

landau commented 6 years ago

@awwright I don't understand how a poor implementation of throwing a ValidationError is relevant to ValidationError not being a "subclass" of Error. Can you please elaborate?

awwright commented 6 years ago

@landau It's poorly implemented, as in, the code wasn't designed to support throwing validation errors. So there's been a lot of problems with trying to use the feature, through we've been fixing them. One of the things missing is, as is pointed out, ValidationError isn't derived from Error.

landau commented 6 years ago

Thank you for the explanation.

the code wasn't designed to support throwing validation errors

It seems to me that the code, specifically this line https://github.com/tdegrunt/jsonschema/blob/038fc524548ae19fd6fa5fb88bb08b9fea087ab6/lib/helpers.js#L51-L53, is designed to throw ValidationError since there is an argument for doing so and the code does throw ValidationError. Perhaps there is something I'm missing around the "poorly implemented" bit.

Getting back on topic, ValidationError doesn't inherit from Error. Can this project implement this?

awwright commented 4 years ago

I think the best way to go about this is adding a new interface, maybe ValidatorResultError, which has a similar interface to ValidatorResult except that it extends from Error. It would contain the first error as the message, and contain the rest of the errors under the errors property.

awwright commented 4 years ago

Gonna bump this up to the next minor (feature) release, it will be enabled with the throwFirst and throwAll options.

awwright commented 4 years ago

This is implemented in 85836efd1b95751674ec820ee62767a7a6632571