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

Matchers: ||, &&, and ! instead of anyOf, allOf, isNot #3727

Closed
DartBot opened this issue Jun 18, 2012 · 12 comments
Closed

Matchers: ||, &&, and ! instead of anyOf, allOf, isNot #3727

DartBot opened this issue Jun 18, 2012 · 12 comments
Labels
area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams. type-enhancement A request for a change that isn't a bug

Comments

@DartBot
Copy link

DartBot commented Jun 18, 2012

This issue was originally filed by @seaneagan


consider:

expect(v, !(isNull || isEmpty));
// vs.
expect(v, isNot(anyOf(isNull, isEmpty)));

expect(v, new isInstanceOf<num> && isPositive);
// vs.
expect(v, allOf(new isInstanceOf<num>, isPositive));

The operators are clearly shorter, but more importantly to me, they read more naturally, and I would think this would be especially true for non-native English speakers.

One small problem... the ! operator is not currently user-definable. Is there a reason for this ?

Could also potentially remove all the composite negation matchers:

isFalse // somewhat misleading since it matches more than just "false".
isNotNull
isNonZero
isNonNegative
isNonPositive

and just have the ! operator of the Matcher they are negating, lazily initialize them:

! isTrue
! isNull
! isZero
! isNegative
! isPositive

It would be unusual to want to override || or && operators, but for the ! operator, it would be useful to customize the negated message, for the description for:

! hasChildren

could be customized to:

"to not have any children"

instead of:

"not <to have children>"

which might be generated by:

isNot(hasChildren)

@lrhn
Copy link
Member

lrhn commented Jun 19, 2012

One small problem... the ! operator is not currently user-definable. Is there a reason for this ?

Nor are || and &&. For those the reason is simple: They are not really operators, but rather a shorthand for more complex control flow (just as the conditional operator). I'm guessing that "!", being the fourth boolean "operator" was made not-user-configurable for consistency with the other three - boolean operators can just not be overridden.

@whesse
Copy link
Member

whesse commented Jun 19, 2012

I would suggest a top-level "not", and member functions "and" and "or".
expect(v, not(isNull.or(isEmpty)))

expect(v, new isInstanceOf<num>.and(isPositive));

@DartBot
Copy link
Author

DartBot commented Jun 19, 2012

This comment was originally written by @seaneagan


@1: makes sense, if || and && were overridable, then the RHS would get evaluated conditionally based on whether the operators were overloaded, which would be unpredictable.

@2: +1, as that reads the exact same way as the operators would, "not" clearly reads betters than "isNot":

not(equals(5))
not(contains(5))
not(startsWith("foo"))
// vs.
isNot(equals(5))
isNot(contains(5))
isNot(startsWith("foo"))

... and "and" and "or" read more fluently than "allOf" and "anyOf":

expect(v, isNull.or(isEmpty))
expect(v, new isInstanceOf<num>.and(isPositive));
// vs.
expect(v, anyOf(isNull, isEmpty))
expect(v, allOf(new isInstanceOf<num>, isPositive));

@anders-sandholm
Copy link
Contributor

Added Area-Library, Triaged labels.

@sethladd
Copy link
Contributor

cc @gramster.

@gramster
Copy link
Contributor

I don't think this will happen before M1, and it comes with some cost. I assume that the expectation is that the base Matcher class will support additional methods for composition:

Match and(Matcher other);
Match or(Matcher other);

This means we need to add extra non-final properties to the Matchers for these, and we can't have const matchers for common cases like we do now (e.g. isTrue).

Also, one thing I don't like about this is you can do weird compositions like:

A.and(B).or(C).and(D).or(E)

@DartBot
Copy link
Author

DartBot commented Feb 27, 2013

This comment was originally written by @seaneagan


I think someone suggested | and & on the mailing list, looks like it didn't make it to this bug. Along with "not", seems pretty nice:

expect(x, not(isNull | isEmpty));
expect(y, not(isNull) & isPositive);
// vs.
expect(x, isNot(anyOf(isNull, isEmpty)));
expect(y, allOf(isNotNull, isPositive));

@lrhn
Copy link
Member

lrhn commented Aug 19, 2013

Removed Type-Defect, Area-Library labels.
Added Type-Enhancement, Area-UnitTest labels.

@kevmoo
Copy link
Member

kevmoo commented Feb 12, 2014

Added Pkg-Unittest label.

@kevmoo
Copy link
Member

kevmoo commented Feb 12, 2014

Removed Area-UnitTest label.
Added Area-Pkg label.

@kevmoo
Copy link
Member

kevmoo commented May 9, 2014

Removed Pkg-Unittest label.
Added Pkg-matcher label.

@DartBot DartBot added Type-Enhancement area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams. labels May 9, 2014
@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

This issue has been moved to dart-lang/matcher#11.

@DartBot DartBot closed this as completed Jun 5, 2015
@kevmoo kevmoo added type-enhancement A request for a change that isn't a bug and removed triaged labels Mar 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams. type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

7 participants