sindresorhus / ow

Function argument validation for humans
https://sindresorhus.com/ow/
MIT License
3.81k stars 108 forks source link

Add multiple error reporting #192

Closed vladfrangu closed 3 years ago

vladfrangu commented 3 years ago

This PR closes #5

What's been added

This PR adds multiple validation error reporting to Arguments and AnyPredicates, by attaching a validationErrors map to the resulting error (heavily simplified). I am open to discussion about possible alternatives, issues, concerns, or just how you would want to see this implemented instead. Personally, I believe this implementation is clean enough (at least from the UX side of things) that it shouldn't be too breaking (however it does classify as semver major). With the current implementation, you may then take the resulting errors and present them however you may want in your API for example. If the full error message is desired to show all errors, let me know and I will look into how that could be displayed in a nicer way

The error stacktrace is also kept, currently looking like this by running the example.js file in the root of the repository

r [ArgumentError]: Multiple errors were encountered. Please check the `validationErrors` property of the thrown error
    at fn (C:\Users\Vlad\Desktop\ow\example.js:13:2)
    at C:\Users\Vlad\Desktop\ow\example.js:17:2
    at logError (C:\Users\Vlad\Desktop\ow\example.js:6:3)
    at Object.<anonymous> (C:\Users\Vlad\Desktop\ow\example.js:16:1)
    at Module._compile (internal/modules/cjs/loader.js:1076:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1097:10)
    at Module.load (internal/modules/cjs/loader.js:941:32)
    at Function.Module._load (internal/modules/cjs/loader.js:782:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:72:12) {
  validationErrors: Map(1) {
    'input' => [
      'Expected `input` to be of type `string` but received type `number`',
      'Expected string `input` to have a minimum length of `10`, got `10`'
    ]
  }
}

In-depth, going file by file


IssueHunt Summary ### Referenced issues This pull request has been submitted to: - [#5: Report multiple type errors](https://issuehunt.io/repos/105227249/issues/5) ---
sindresorhus commented 3 years ago

This change will need a lot more tests.

vladfrangu commented 3 years ago

This change will need a lot more tests.

@sindresorhus I think these tests should cover almost everything that is needed! Of course, however, if you have any other edge cases you may want me to test, please let me know! πŸ˜‹

I tried to cover every code path possible, and, as coverages report, all are covered

vladfrangu commented 3 years ago

I merged master into this PR, let me know if there's anything you'd like changed πŸ™

sindresorhus commented 3 years ago

If the full error message is desired to show all errors, let me know and I will look into how that could be displayed in a nicer way

Per the original issue. The error is meant for humans, so it should present all the errors by default.

sindresorhus commented 3 years ago

Sorry for the slow reply. I'm just finally now getting through my long list of notifications.

vladfrangu commented 3 years ago

Sorry for the slow reply. I'm just finally now getting through my long list of notifications.

Don't worry, I would've assumed that was the case! It's totally understandable

vladfrangu commented 3 years ago

There we go @sindresorhus, the errors outputted are now in one big message, while also keeping the validationErrors property public so users can present it any way they wish

ArgumentError: Expected `input` to be of type `string` but received type `number`
Expected string `input` to have a minimum length of `10`, got `10`
Expected string `input` to end with `owo`, got `10`
    at ow (C:\Users\Vlad\Desktop\ow\dist\index.js:21:23)
    at fn (C:\Users\Vlad\Desktop\ow\example.js:13:2)
    at C:\Users\Vlad\Desktop\ow\example.js:17:2
    at logError (C:\Users\Vlad\Desktop\ow\example.js:6:3)
    at Object.<anonymous> (C:\Users\Vlad\Desktop\ow\example.js:16:1)
    at Module._compile (internal/modules/cjs/loader.js:1063:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1092:10)
    at Module.load (internal/modules/cjs/loader.js:928:32)
    at Function.Module._load (internal/modules/cjs/loader.js:769:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:72:12) {
  validationErrors: Map(1) {
    'input' => [
      'Expected `input` to be of type `string` but received type `number`',
      'Expected string `input` to have a minimum length of `10`, got `10`',
      'Expected string `input` to end with `owo`, got `10`'
    ]
  }
}
sindresorhus commented 3 years ago

Seems like the code coverage is below the threshold.

vladfrangu commented 3 years ago

Seems like the code coverage is below the threshold.

That's..confusing to say the least.. I can't even see why on codecov, and on local npm run test, I get this

Local Coverage report ``` -------------------------------------|---------|----------|---------|---------|------------------- File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s -------------------------------------|---------|----------|---------|---------|------------------- All files | 99.48 | 94.27 | 93.03 | 99.46 | source | 99.28 | 94.74 | 59.49 | 99.26 | argument-error.ts | 100 | 66.67 | 100 | 100 | 9 index.ts | 100 | 100 | 30.43 | 100 | modifiers.ts | 100 | 100 | 100 | 100 | predicates.ts | 98.55 | 100 | 68.63 | 98.55 | 303 test.ts | 100 | 100 | 100 | 100 | source/operators | 100 | 100 | 100 | 100 | not.ts | 100 | 100 | 100 | 100 | source/predicates | 100 | 98.28 | 100 | 100 | any.ts | 100 | 100 | 100 | 100 | array-buffer.ts | 100 | 100 | 100 | 100 | array.ts | 100 | 100 | 100 | 100 | base-predicate.ts | 100 | 100 | 100 | 100 | boolean.ts | 100 | 100 | 100 | 100 | data-view.ts | 100 | 100 | 100 | 100 | date.ts | 100 | 100 | 100 | 100 | error.ts | 100 | 100 | 100 | 100 | map.ts | 100 | 100 | 100 | 100 | number.ts | 100 | 100 | 100 | 100 | object.ts | 100 | 100 | 100 | 100 | predicate.ts | 100 | 96.97 | 100 | 100 | 87 set.ts | 100 | 100 | 100 | 100 | string.ts | 100 | 100 | 100 | 100 | typed-array.ts | 100 | 100 | 100 | 100 | weak-map.ts | 100 | 100 | 100 | 100 | weak-set.ts | 100 | 100 | 100 | 100 | source/utils | 97.66 | 92.86 | 100 | 97.5 | generate-argument-error-message.ts | 100 | 100 | 100 | 100 | generate-stack.ts | 100 | 100 | 100 | 100 | has-items.ts | 100 | 100 | 100 | 100 | infer-label.ts | 90.32 | 92 | 100 | 90 | 25,33,41 is-valid-identifier.ts | 100 | 100 | 100 | 100 | match-shape.ts | 100 | 86.36 | 100 | 100 | 55-58,91 of-type-deep.ts | 100 | 100 | 100 | 100 | of-type.ts | 100 | 100 | 100 | 100 | random-id.ts | 100 | 100 | 100 | 100 | source/utils/node | 100 | 75 | 100 | 100 | is-node.ts | 100 | 75 | 100 | 100 | 1 -------------------------------------|---------|----------|---------|---------|------------------- ```

A lot of these aren't even caused by me... Maybe I should try a rebase?

vladfrangu commented 3 years ago

@sindresorhus back again, after a rebase (technically it was squash, commit, rebase, commit, commit, squash, commit), seems like everything is back in order! Probably my local branch was outdated compared to your master and caused codecov to not report right. πŸ‘

sindresorhus commented 3 years ago

Nice work. Thanks ;)