Closed GoogleCodeExporter closed 8 years ago
I would prefer to see it on an abstract class called something like
ChainedMatcher.
Otherwise you are requiring anyone who implements a Matcher to implement the
two methods.
You could even push and() and or() into their own interface called Chainable<?
blah
Matcher<?>> so you'd have ChainableMatcher<T> extends BaseMatcher<T> implements
Chainable<T>. (Where <? blah Matcher<?>> is the appropriate incantation to get
it to
compile).
Of course then you have the combinatorial thing where you might want a
TypeSafeChainableMatcher<<<??>>>. Perhaps BaseMatcher could implement Chainable.
I definitely don't think and/or belong in Matcher though.
Original comment by tasta...@gmail.com
on 15 Apr 2008 at 4:37
The supposed patch DOES NOT require anyone to implement the two methods,
because they
are already implemented in BaseMatcher and therefore inherited by all other
matchers.
Original comment by michael.tamm2
on 16 Apr 2008 at 6:45
On the other hand, if and()/or() are only in BaseMatcher, we'd have to pass
concrete types around more often.
One of the nice features about Hamcrest is that most of the time Matcher is all
you need. After all, BaseMatcher is
supposed to be a bit of helper infrastructure, not part of the API.
Original comment by smgfree...@gmail.com
on 16 Apr 2008 at 10:41
@smgfreeman: I hope I did not missunderstand your comment - I did not suppose,
that
the methods "and(...)" and "or(...)" are only in the BaseMatcher: These two
methods
should of course be added to the interface Matcher - so there is no need to pass
around concrete types.
I just wanted to point out, that if those two method are added to the Matcher
interface, it is not true, that anyone needs to implement these methods: The
implementation can be provided in BaseMatcher, which is the base class for all
other
matchers.
Original comment by michael.tamm2
on 16 Apr 2008 at 12:20
I understand. I was responding to Dan.
Original comment by smgfree...@gmail.com
on 16 Apr 2008 at 12:35
This should be implemented as builders layered above the matcher API. The
matcher
interface is *not* the right place for and and or because:
# Breaks Layering: the Matcher interface defines objects that implement the
matching. Other classes (e.g. Matchers) define syntactic sugar for
declaratively
expressing what the match means. See http://www.jmock.org/oopsla2006.pdf for a
description of the architectural style.
# Feature Creep: the Matcher interface is intended to define a simple,
self-describing predicate, not define how those predicates can be combined.
# Lack of Uniformity: why only and and or? What about not? What about all the
other boolean operators (if, iff, xor, parity, etc?)
# Ambiguity. Without brackets, expressions are ambiguous. What does
m.and(x).or(y).and(z) mean? Do and and or have the same precedence as the
short-cut
logical operators in Java? Or do they have the same precedence as method
calls?
*There is no correct answer.* Either option will confuse a significant number
of
users. Therefore, the Matcher interface is the wrong place to put this.
Hamcrest is *not* designed to replicate English sentences in Java code. It's
designed to be a simple, extensible set of matchers that can be combined and
easily
used by other projects. We have made an effort to provide syntactic sugar so
that
matchers can be used with a terse, readable, extensible, unambiguous notation.
But
that is a secondary goal of the project.
The ideal solution would be to implement combinations of matchers as a separate
set
of builder classes. The API would look something like the following (but with
a
better name than combine if you can think of one):
Matcher all = combine(m1).and(m2).and(m3);
Matcher any = combine(m1).or(m2).or(m3);
Then you can use static typing to ensure that infix combinations are
unambiguous.
E.g. You could not write the ambiguous statement I used above, but would have
to
combine and and or like the following, so that the brackets show the precedence
explicitly:
combine(m1).and(combine(m2).or(m3));
The implementation would look something like this (except I've not put the
visibility
or generic declarations because I'm lazy):
class Combine {
AndMatcher and(Matcher m) { ... }
OrMatcher or(Matcher m) { ... }
static Combine combine(Matcher m) { return new Combine(m); }
}
class AndMatcher implements Matcher {
AndMatcher and(Matcher m) { ... }
}
class OrMatcher implements Matcher {
OrMatcher or(Matcher m) { ... }
}
Original comment by nat.pr...@gmail.com
on 16 Apr 2008 at 12:42
Nat is on the right track. It is most sensible to build these sugary components
outside of the Mathcer interface.
Having said that, we could consider changing the static factory code generator
to
automatically returned instances of ChainedMatcher such that one can write
equals(x).or(equals(y)).
I also agree with making the builders for And and Or seperate such that you can
only
go down one route at the top level of the chaining. This makes precedence a bit
easier.
If we can come to consensus I'll build this.
Original comment by neild...@gmail.com
on 22 Apr 2008 at 11:54
It's been some time since I started this issue. In the meanwhile I come to like
the
solution provided by JUnit 4.x: They have an AndMatcher as well as an OrMatcher
and
provide access to them by the static methods both and either defined in the
class
JUnitMatchers -
http://junit.org/junit/javadoc/4.5/org/junit/matchers/JUnitMatchers.html
This way you can write:
assertThat(string, both(containsString("a")).and(containsString("b")));
or:
assertThat(string, either(containsString("a")).or(containsString("b")));
I think the Hamcrest project could adapt this solution.
Original comment by michael.tamm2
on 26 Aug 2008 at 3:21
Agreed. That sounds good, except 'both' only works when there are two combined
matchers.
Original comment by nat.pr...@gmail.com
on 2 Oct 2008 at 8:21
Original comment by nat.pr...@gmail.com
on 2 Oct 2008 at 8:22
Absorbed the CombinatorialMatcher from JUnit into core.
Original comment by smgfree...@gmail.com
on 12 Oct 2008 at 11:23
Original comment by smgfree...@gmail.com
on 22 Nov 2008 at 12:18
Original issue reported on code.google.com by
michael.tamm2
on 15 Apr 2008 at 3:43Attachments: