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

Invert only the following predicate with the 'not' operator #71

Merged
merged 4 commits into from
Jun 21, 2018

Conversation

Apostrophii
Copy link
Contributor

This PR changes the not operator from inverting all of the following predicates to only inverting the single following predicate.

ow(1, ow.number.not.infinite.not.greaterThan(5)) // will not throw

ow(1, ow.number.not.infinite.greaterThan(5)) // will throw

As ow is designed for human readability I believe that this is a more natural API (at least for English speakers) as the above code would be read

1 is a number and not infinite and not greater than 5.

and

1 is a number and not infinite and greater than 5.

respectively.

@sindresorhus
Copy link
Owner

I was against this when we first added not (#12 (comment)), but I'm leaning towards it now. You have a good point.

@SamVerschueren Thoughts?

@sindresorhus sindresorhus mentioned this pull request May 24, 2018
87 tasks
@sindresorhus sindresorhus mentioned this pull request Jun 7, 2018
@SamVerschueren
Copy link
Collaborator

Makes sense. I'm ok with this change. Can you rebase against master?

@sindresorhus
Copy link
Owner

@SamVerschueren Should we remove string.nonEmpty? Since people can just do string.not.empty now.

@SamVerschueren
Copy link
Collaborator

I would be ok with removing it, although the error message might be slightly better with nonEmpty.

[NOT] Expected string to be empty, got ``

Expected string to not be empty

@sindresorhus
Copy link
Owner

@SamVerschueren Right now, yes, but the intention is to improve the negated errors messages. If we keep nonEmpty, we need it for other predicates too, to be consistent.

@SamVerschueren
Copy link
Collaborator

When I started implementing not, I was thinking about rephrasing messages automatically. I thought, If I always use to be, we could change that to not be. But then I had error messages where I couldn't use to be. So if we want clean messages for not, we might want to add an invertedMessage property to the validation object.

I'm not saying we shouldn't drop nonEmpty, was just giving an argument why we might want to keep it (for now).

@sindresorhus
Copy link
Owner

@SamVerschueren Let's continue this over at #101 (Can you copy your above comment in there?)

@sindresorhus sindresorhus merged commit c1de125 into sindresorhus:master Jun 21, 2018
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.

None yet

3 participants