Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add any predicate #56

Merged
merged 3 commits into from
Mar 14, 2018
Merged

Add any predicate #56

merged 3 commits into from
Mar 14, 2018

Conversation

SamVerschueren
Copy link
Collaborator

@SamVerschueren SamVerschueren commented Mar 9, 2018

Added the any method so you can write.

ow(1, ow.any(ow.string, ow.number));

I added typings for any with up to 8 parameters. The benefit is that the compiler can now also check (in the previous example) that 1 should be of type string or number. So we get compile-time checking as well out of the box when we use 8 or less parameters.


Secondly, I refactored the main Ow function. Now the predicate checks are done in the concrete predicate class itself. I believe this will make extending Ow much easier as we could now create an OptionalPredicate class that has it's own test function seperate from the Predicate and AnyPredicate class. Just check the code, hard to explain and I hope it makes sense :p.

source/index.ts Outdated
@@ -25,6 +26,18 @@ export interface Ow {
* @param predicate Predicate used in the validator function.
*/
create<T>(predicate: Predicate<T>): (value: T) => void;
/**
* Test the value for multiple predicates.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about the description.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test that the value matches at least one of the given predicates.

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, much better!

@sindresorhus
Copy link
Owner

I added typings for any with up to 8 parameters.

Oh wow. That reminds me of https://github.com/Microsoft/TypeScript/blob/master/lib/lib.es2015.promise.d.ts#L41-L113

We really need microsoft/TypeScript#5453

@sindresorhus
Copy link
Owner

Did you pick 8 for a reason? Can we make it 10? Still arbitrary, but feels better.

@SamVerschueren
Copy link
Collaborator Author

Hehe, you're right, 10 is better. I just thought 8 was crazy enough.

@sindresorhus
Copy link
Owner

Secondly, I refactored the main Ow function. Now the predicate checks are done in the concrete predicate class itself. I believe this will make extending Ow much easier as we could now create an OptionalPredicate class that has it's own test function seperate from the Predicate and AnyPredicate class.

Yeah, makes sense.

@sindresorhus sindresorhus merged commit 7bbfee0 into master Mar 14, 2018
@sindresorhus sindresorhus deleted the any-predicate branch March 14, 2018 17:12
@kevva
Copy link
Contributor

kevva commented Mar 22, 2018

Might be a stupid question, but why couldn't this be ow(1, [ow.string, ow.number]);? I guess because of TypeScript 👼?

@sindresorhus
Copy link
Owner

sindresorhus commented Mar 23, 2018

@kevva Readability too. It's not clear whether it means any or all. In the above case it's clear as those are mutually exclusive, but what about:

ow(unicorn, [ow.string.startsWidth('foo'), ow.string.endsWidth('bar')]);

Does it require a string that both starts with foo and ends with bar, or a string that either starts with foo or ends with bar.

@kevva
Copy link
Contributor

kevva commented Mar 23, 2018

Yeah, sounds reasonable. On a similar note, wouldn't accepting an array in any fix the typings issue though?

@sindresorhus
Copy link
Owner

What typing issue?

@kevva
Copy link
Contributor

kevva commented Mar 23, 2018

The one where we have to use an arbitrary number like 10 for the arguments :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants