Skip to content

Conversation

leaumar
Copy link

@leaumar leaumar commented May 16, 2018

This simple addition allows users on java 8 to do this:

FeatureMatcher.ofFunction(Thing::calculateName, containsString("foo"), "calculated name contains foo", "calculateName")

instead of

new FeatureMatcher(containsString("foo"), "calculated name contains foo", "calculateName") {
  @Override
  protected String featureValueOf(Thing actual) {
    return actual.calculateName();
  }
}

Less verbose/more concise, more pleasant to work with, and does not require hamcrest to be updated to java 8 or anything.

@nhojpatrick
Copy link
Member

@leaumar looking at getting java 8, 11 and newer support into hamcrest, please can you rebase from master, as hamcrest-core and hamcrest-library have been refactored a lot and also deprecated, so that everything is just in hamcrest.

@nhojpatrick
Copy link
Member

Going to try and kick start hamcrest, so if you want to get it merged, please rebase from the branch feature/v3.x-ea.
Still trying to understand how has permissions to perform a release.
Looking to keep v2.x as java 7, with v3 bumping to java 8.

@rmcdouga
Copy link

This PR (as it currently stands) will have some problems. I am working on similar code for FeatureMatcher and TypeSafeDiagnosingMatcher however the code runs afoul of the unit tests when actually used (specifically the AbstractMatcherTest.testCopesWithUnknownTypes() test).

The issue is that the code throws a ClassCastException when passed an object that doesn't match the expected type. The failing AbstractMatcherTest is testing that this case is handled quietly (without an exception) and just fails the match. This functionality is supposed to be provided by TypeSafeDiagnosingMatcher however that code uses runtime reflection to detect a type mismatch. This is defeated by type erasure because the static constructor uses generics. I assume the use of reflection is to avoid causing and then catching an exception which could be expensive (which would slow down the tests).

I am working to see if there's a way to improve the reflection code to handle the generics as I think this addition is worthwhile.

(On a side note, this demonstrates why it's a code smell to have a code change without a corresponding test change - in this case, adding a test that exercises the static constructor would have surfaced the issue.)

@rmcdouga
Copy link

I've created a pull request that I believe supersedes this one. See #441 . I think this request should be closed without merging it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Needs triage
Development

Successfully merging this pull request may close these issues.

3 participants