sindresorhus / ow

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

Fix input type on some methods and add `AssertingValidator` #224

Closed cpiber closed 2 years ago

cpiber commented 2 years ago

Fixes #222

I am not fully sure about the last two points, suggestions welcome.

cpiber commented 2 years ago

@sindresorhus Input?

cpiber commented 2 years ago

While this isn't resolved, you can use

const is = <T>(v: unknown, pred: BasePredicate<T>): v is T => ow.isValid(v, pred);

to substitute ow.isValid

cpiber commented 2 years ago

For some reason, I cannot reply to the review, so here:

Shouldn't the asserts be added to ReusableValidator?

I tried that, but then you get Assertions require every name in the call target to be declared with an explicit type annotation.ts(2775) as mentioned before

Why not improve the exactShape type instead of adding a separate type here?

First of all, I didn't want to cause breaking changes, and second, I don't know if it is possible to do that, and if it is how. As I said, I introduced that type to make sure TypeScript actually verifies, that the shape of the argument and the type of what I want to verify against are the same, since simply declaring the target variable as ObjectValidator<> does not work. Please do suggest a better solution.

sindresorhus commented 2 years ago

I tried that, but then you get Assertions require every name in the call target to be declared with an explicit type annotation.ts(2775) as mentioned before

I would try to investigate that. Maybe there's a workaround. I don't have time to look into these issues now.

First of all, I didn't want to cause breaking changes, and second, I don't know if it is possible to do that, and if it is how. As I said, I introduced that type to make sure TypeScript actually verifies, that the shape of the argument and the type of what I want to verify against are the same, since simply declaring the target variable as ObjectValidator<> does not work. Please do suggest a better solution.

I unfortunately don't have time to look for a better solution.

I suggest removing the unrelated changes for now and focus on "Some functions incorrectly used value: T instead of value: unknown (want to assert value is T, not assume it)". The other things can be done later on in a separate pull request or opened as an issue instead if you cannot find a good solution for it.

cpiber commented 2 years ago

I would try to investigate that. Maybe there's a workaround. I don't have time to look into these issues now.

I don't think it's possible due to the nature of ow.create: TS(2775) requires the variable to which ow.create's return is saved to to be explicitly typed from what I understand, I already tried to search for a better solution.

I suggest removing the unrelated changes for now

Understood, I'll remove them in a sec

sindresorhus commented 2 years ago

AssertingValidator should not be exported to the user. It's only meant for the tests here.

cpiber commented 2 years ago

Is it? In my opinion, it should be exported, so that the user can have a validator, that actually asserts

sindresorhus commented 2 years ago

The intention for that was not clear. Then it needs a proper doc comment with a description (including why it exists) and a usage example.