hardayal / hamcrest

Automatically exported from code.google.com/p/hamcrest
0 stars 0 forks source link

Prevent either().and() and both().or() #154

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago
I suggest replacing

    @Factory
    public static <LHS> CombinableMatcher<LHS> both(Matcher<? super LHS> matcher) {
        return new CombinableMatcher<LHS>(matcher);
    }

by

    @Factory
    public static <LHS> CombinableMatcher<LHS> both(Matcher<? super LHS> matcher) {
        return new CombinableMatcher<LHS>(matcher) {
            @Override
            public CombinableMatcher<LHS> or(Matcher<? super LHS> other) {
                throw new UnsupportedOperationException("both().or() is an invalid combination, use either().or() instead");
            }
        };
    }

and

    @Factory
    public static <LHS> CombinableMatcher<LHS> either(Matcher<? super LHS> matcher) {
        return new CombinableMatcher<LHS>(matcher);
    }

by

    @Factory
    public static <LHS> CombinableMatcher<LHS> either(Matcher<? super LHS> matcher) {
        return new CombinableMatcher<LHS>(matcher) {
            @Override
            public CombinableMatcher<LHS> and(Matcher<? super LHS> other) {
                throw new UnsupportedOperationException("either().and() is an invalid combination, use both().and() instead");
            }
        };
    }

Without this change both() and either() are identical and the combinations 
both().or() and either().and() are possible whereby the first verb is 
absolutely insignificant. They change I proposed makes it still possible to use 
the CombinableMatcher stand-alone like it is. The only code that will break by 
this change is the usage of either().and() or both().or() and both of them are 
most probably not what was intended and at least make the assertion less 
meaningful and confusing.

Original issue reported on code.google.com by vampi...@gmail.com on 30 May 2011 at 2:39

GoogleCodeExporter commented 8 years ago
Maybe the change could/should also be synchonized with the according change to 
JUnit if there will be: https://github.com/KentBeck/junit/issues/236

Original comment by vampi...@gmail.com on 31 May 2011 at 7:45

GoogleCodeExporter commented 8 years ago
Created a patch for this issue / feature.
Linked with issue / feature 236 from junit on github: 
https://github.com/KentBeck/junit/pull/272.

Note that I couldn't run the entire test suite, cause it's giving me some wired 
errors. I'll try to run it soon.

Original comment by marvinwa...@gmail.com on 24 Jul 2011 at 3:22

Attachments:

GoogleCodeExporter commented 8 years ago
I got a Windows machine, applyed the patch and runned the entire test suite and 
all tests passed.

Original comment by marvinwa...@gmail.com on 25 Jul 2011 at 5:15

GoogleCodeExporter commented 8 years ago
tagging

Original comment by t.denley on 12 May 2012 at 10:34

GoogleCodeExporter commented 8 years ago

Original comment by t.denley on 12 May 2012 at 12:13

GoogleCodeExporter commented 8 years ago
I have addressed this issue by making it compilationally impossible to do 
both(x).or(y) or either(x).and(y).  See the following commit on github: 
https://github.com/hamcrest/JavaHamcrest/commit/1c33be116091a885b21797aae761d565
2bcdf3af

Please verify this fix.

Original comment by t.denley on 12 May 2012 at 1:28

GoogleCodeExporter commented 8 years ago

Original comment by t.denley on 19 May 2012 at 2:51