| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Thanks for this, @seregamorph! I appreciate the time you've put into this PR. The code is clean and readable, and I think it will be a valuable addition to Hamcrest. If you don't mind, can I request some changes? Firstly, I'm not keen on the use of anonymous inner classes for published matchers in Hamcrest. It would be great if you re-write them as top level public classes that subclass the same TypeSafeDiagnosingMatcher<Optional<T>> parent Secondly, I'm not definite about this, but I think it would be nicer to put them in a separate sub-package (e.g. optional) Thirdly (and finally), I think static factory method names could give a bit more context, to make them more readable when they've been staticly imported. e.g. change isEmpty() to emptyOptional(), and change isPresent() to optionalWithValue(). Putting those points together, we'd end up with something like this: import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.not;
import static org.hamcrest.optional.OptionalMatchers.emptyOptional;
import static org.hamcrest.optional.OptionalMatchers.optionalWithValue;
import static org.hamcrest.text.MatchesPattern.matchesPattern;
import org.junit.Test;
import java.util.Optional;
public class OptionalMatchersTest {
@Test
public void testEmptyOptional() {
Optional<Object> actual = Optional.empty();
// assertThat(actual, isEmpty());
assertThat(actual, is(emptyOptional()));
assertThat(actual, not(optionalWithValue()));
}
@Test
public void testOptionalWithValue() {
Optional<String> actual = Optional.of("Hello, world");
// assertThat(actual, isPresent());
assertThat(actual, not(emptyOptional()));
assertThat(actual, is(optionalWithValue()));
assertThat(actual, optionalWithValue("Hello, world"));
assertThat(actual, optionalWithValue(matchesPattern("Hell")));
}
}What do you think? |
Sorry, something went wrong.
|
Thank you for the review and comments. Done:
Also please note: I adjusted a bit the description message from "to be empty"/"to be present"/"to be present and match" to "empty"/"present"/"present and matches" because of the notation you suggested with "is" wrapper. These tests clarify the failure messages: AssertionError failure = assertThrows(AssertionError.class, () -> {
assertThat(Optional.of(1), is(emptyOptional()));
});
assertEquals("\n" +
"Expected: is empty\n" +
" but: is Optional[1]", failure.getMessage());or without is wrapper: AssertionError failure = assertThrows(AssertionError.class, () -> {
assertThat(Optional.of(1), emptyOptional());
});
assertEquals("\n" +
"Expected: empty\n" +
" but: is Optional[1]", failure.getMessage()); |
Sorry, something went wrong.
|
Thanks again @seregamorph. Now that I can see the changes in context, I have a bit more feedback:
|
Sorry, something went wrong.
|
Done |
Sorry, something went wrong.
There was a problem hiding this comment.
Looks good, thanks!
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
This change was waiting for releasing 3.0 with JDK 8 compatibility.
Matchers for java.util.Optional: