atlassian / better-ajv-errors

JSON Schema validation for Human 👨‍🎤
https://atlassian.github.io/better-ajv-errors/
Other
232 stars 44 forks source link

Feature request: Filter out 'shadowing' errors, for the humans #76

Open nicojs opened 4 years ago

nicojs commented 4 years ago

Hi there 👋. Awesome repo you've got here!

We at the stryker mutation testing team have been looking into validation with ajv. We've come to the same conclusion, error messages directly from AJV are not really designed for humans. We're thinking of using better-ajv-errors.

However, we're also thinking of filtering out what I call 'shadowing' errors.

A shadowing error is an error that results logically from another error. Some examples:

[
  { // This is a useless error for a human
   keyword: 'type',
   dataPath: '.mutator',
   params: { type: 'string' },
   // [...]
 },
 { // => This is the most specific error. This is for humans!
   keyword: 'required',
   dataPath: '.mutator',
   params: { missingProperty: 'name' },
   // [...]
 },
 { // This is a useless error for a human
   keyword: 'oneOf',
   dataPath: '.mutator',
   params: { passingSchemas: null },
   [...]
 }
]

Or:

[
  { // This is a useless error for a human
   keyword: 'type',
   dataPath: '.logLevel',
   params: { type: 'string' },
   // [...]
 },
 { // => This is the most specific error. This is for humans!
   keyword: 'enum',
   dataPath: '.logLevel',
   params: { allowedValues: ['info', 'warn'] },
   // [...]
 },
]

A first draft of the filtering is created here:

https://github.com/stryker-mutator/stryker/blob/627d2f7e403042845bcece73c838c9447bbf522c/packages/core/src/config/validationErrors.ts#L55-L74

Do you think this filtering is useful for other projects as well? Should it be added to better-ajv-errors? Maybe as an option? Or do you want to keep it separate?

I would be willing to prepare it in a PR if you agree that this is a feature useful for all humans, nut just mutant-killing humans 😉.

torifat commented 4 years ago

Sounds interesting, happy to give it a try as an option.

Extremely sorry about the late reply. I wasn't active for a while.

nicojs commented 4 years ago

Thanks for the response. It is late, but I don't have any illusion of entitlement, no problem whatsoever 😊

We've implemented the error reporting without better-ajv-errors at the moment, but we're happy to move to better-ajv-errors when this feature is implemented.

The current filtering can be found in our code here: https://github.com/stryker-mutator/stryker/blob/2eec539ddaffa9fd65eaf1413ab29885a45ea510/packages/core/src/config/validationErrors.ts#L55-L76

Do you agree with this filtering? If so, I'd be happy to move it to here and add the necessary tests for it as well.

torifat commented 4 years ago

Yes, I totally understand why they are irrelevant. I just don't want to make that a default option right away 🙂

nicojs commented 4 years ago

How about adding it as an option? Maybe { filterShadowingErrors: true } (default false)?