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 not predicate #12

Merged
merged 3 commits into from
Nov 1, 2017
Merged

Add not predicate #12

merged 3 commits into from
Nov 1, 2017

Conversation

SamVerschueren
Copy link
Collaborator

@SamVerschueren SamVerschueren commented Oct 18, 2017

Looking into negating predicates. This PR adds the following syntax.

ow(10, m.number.not.infinite);

I noticed 2 suggestion in the validator ideas.

  • not (Inverts the following predicates)
  • not(type) (Inverts just the argument) (Example: ow.string.not(ow.empty))

This is not possible to implement. We can not have get not() together with not(predicate: Predicate) in one class.
Also for the not(type) example, it would look like ow.string.not(ow.string.empty). It's perfectly possible to restrict the argument to only being a StringPredicate or NumberPredicate depending on the chain, but .empty lives inside the StringPredicate so has to be accessed via ow.string. Something like ow.string.not(x => x.empty) where x is of type StringPredicate would be possible.

However, we might reconsider the current implementation. Maybe we should just let it negate the first following predicate.

ow(10, m.number.not.infinite.greaterThan(5));

To me, this reads like

10 is of type number, not infinite and greater than 5

ow(1, m.number.not.infinite.not.greaterThan(5));

10 is of type number, not infinite and not greater than 5

Just throwing some ideas here though. Feel free to reject this for now. Going to think about this a little bit more as well. Not entirely keen on the way it's implemented.

@sindresorhus
Copy link
Owner

not(type) (Inverts just the argument) (Example: ow.string.not(ow.empty))

We could give it a different name, like invert or notType.

Maybe we should just let it negate the first following predicate.

Not a big fan of that. It tries to make it too "English". And it makes it positional. The point is that each property is a modifier of everything. The position doesn't matter.

ow(10, m.number.not.infinite.greaterThan(5));

is just meant as a shortcut for

ow(10, m.number.notType(ow.infinite.greaterThan(5)));

@SamVerschueren
Copy link
Collaborator Author

We could give it a different name, like invert or notType.
👌

Any preference?

Not a big fan of that. It tries to make it too "English". And it makes it positional. The point is that each property is a modifier of everything. The position doesn't matter.

Makes sense!

Regarding the notType/invert, we will have to repeat ow.<predicate> though. But we can perfectly restrict the type of the parameter so that we can only provide a NumberPredicate in a number notType call.

ow(10, ow.number.notType(ow.number.infinite.greaterThan(5)));

@SamVerschueren
Copy link
Collaborator Author

I also refactored the code and used a Proxy now. I believe this is a more elegant way of solving the problem :). Feel free if you think the previous implementation is more straightforward. It was just nice to experiment with Proxy :p.

One thing I don't handle currently, is negating the error message.

ow(1, ow.number.not.lessThan(5))
//=> Expected 1 to be less than 5

Unless we specify a negated error message ourselves, I can only think of one way of solving this. By prefixing the error message with [NOT] or something. But it might look weird

[NOT] Expected 1 to be less than 5

@SamVerschueren
Copy link
Collaborator Author

We could give it a different name, like invert or notType.

I thought about this again, maybe I like invert or something like notPredicate more than notType. It feels like notType is more to check if it's not of type number or string or something. Like ow.notType(m.string).

This was referenced Oct 22, 2017
@sindresorhus
Copy link
Owner

I also refactored the code and used a Proxy now. I believe this is a more elegant way of solving the problem :). Feel free if you think the previous implementation is more straightforward. It was just nice to experiment with Proxy :p.

Proxy is slightly an overkill here, but it does make the inverted thing less coupled. I could go either way, so let's go with Proxy since you prefer it. I'm surprised the test pass on Node.js 4 though? It doesn't support Proxy. Does TS try to polyfill proxies or something?

By prefixing the error message with [NOT] or something. But it might look weird

Let's just do this for now. We can improve the error messages later on.

@SamVerschueren
Copy link
Collaborator Author

Like I said, if you find the first implementation more straightforward, happy to revert it. It was just nice to experiment with Proxy :).

You're right, I don't think it works on Node.js 4. If that is a prerequisite, we should probably have to revert the Proxy thing.

@sindresorhus
Copy link
Owner

sindresorhus commented Oct 30, 2017

Then I think we should revert. It would be nice to be able to use this in some libs that have to stay Node.js 4 compatible, like AVA and XO.

@sindresorhus
Copy link
Owner

I thought about this again, maybe I like invert or something like notPredicate more than notType. It feels like notType is more to check if it's not of type number or string or something. Like ow.notType(m.string).

Let's defer this to later. It's not an important feature.

@SamVerschueren
Copy link
Collaborator Author

Then I think we should revert. It would be nice to be able to use this in some libs that have to stay Node.js 4 compatible, like AVA and XO.

I agree! I will have a look to make sure it keeps working on Node 4 👍 .

@SamVerschueren
Copy link
Collaborator Author

I refactored the not operator code to not using proxies. So this implementation works in Node.js 4 as well.

* @param predictate Predicate to wrap inside the operator.
*/
export const not = <T extends Predicate>(predicate: T) => {
predicate['addValidator'] = (validator: Validator<any>) => { // tslint:disable-line:no-string-literal
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because addValidator is marked protected, I had to use the predicate['addValidator'] workaround.

Another thing we can do, which I believe is better, is by not marking addValidator with protected but with an @internal comment. This way, it doesn't end up in the type definitions and we don't have to use this workaround.

Copy link
Owner

Choose a reason for hiding this comment

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

Another thing we can do, which I believe is better, is by not marking addValidator with protected but with an @internal comment. This way, it doesn't end up in the type definitions and we don't have to use this workaround

Sounds good!

@sindresorhus sindresorhus merged commit 494cc40 into master Nov 1, 2017
@sindresorhus sindresorhus deleted the not branch November 1, 2017 14:37
@sindresorhus
Copy link
Owner

Looking good now :)

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.

2 participants