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.

Sign up for GitHub

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

Jump to bottom

Make IsBlank matcher consistent with String.isBlank #326

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
+32 −5
Open

Make IsBlank matcher consistent with String.isBlank #326

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cd267b3
Adds additional unit test that checks all characters considered 'whit…
Thorn1089 Feb 5, 2021
6d5f972
Changes IsBlank matcher to use String.isBlank
Thorn1089 Feb 5, 2021
fbae71b
Fixes checkstyle violation; replaces tabs with spaces
Thorn1089 Feb 5, 2021
c802ff7
Replaces isBlank implementation
Thorn1089 Feb 19, 2021
62e747d
Removes unavailable #isBlank method from test
Thorn1089 Feb 19, 2021
620bbbc
Adds spaces to match style guide
Thorn1089 Feb 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions hamcrest/src/main/java/org/hamcrest/text/IsBlankString.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@
import org.hamcrest.Matcher;
import org.hamcrest.TypeSafeMatcher;

import java.util.regex.Pattern;

import static org.hamcrest.core.AnyOf.anyOf;
import static org.hamcrest.core.IsNull.nullValue;

Expand All @@ -18,13 +16,20 @@ public final class IsBlankString extends TypeSafeMatcher<String> {
@SuppressWarnings("unchecked")
private static final Matcher<String> NULL_OR_BLANK_INSTANCE = anyOf(nullValue(), BLANK_INSTANCE);

private static final Pattern REGEX_WHITESPACE = Pattern.compile("\\s*");

private IsBlankString() { }

@Override
public boolean matchesSafely(String item) {
return REGEX_WHITESPACE.matcher(item).matches();
final int length = item.length();
int offset = 0;
while(offset < length) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider sticking to the Hamcrest code style, which has a space before if and opening parenthesis.
It's not enforced in the build apparently, but still...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can do. Are you using an autoformatter like spotless? It has Maven/Gradle plugins. Might be worth pulling into a separate PR :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing.
I was talking about a checker rather than a formatter.
JavaHamcrest is already using CheckStyle, but apparently it doesn't check much besides the fact that there are no tabs.
For the record: I'm not an official contributor to JavaHamcrest, I'm just a random someone with a stake in the project like you, I don't have the power to merge this PR even if I wanted to.

I imagine you inspired your implementation on Java 11 JDK's sources?
They look similar, yet a little bit different, and I wonder why...

The JDK's implementation distinguishes between Latin-1 and UTF-16 encoded strings. From what I can tell a String can be encoded in either of these 2. Your implementation only reflects the JDK's UTF-16 implementation. Is it safe to assume UTF-16? Does that also cover Latin-1? If such an assumption were safe, why does the JDK's implementation bother to make a difference?

In principle, I agree with this change, I'm just wondering if we should hold off fixing until JavaHamcrest bumps source/target compatibility to Java 11, in which case the fix will become simpler (i.e. your first commit).

final int codePoint = item.codePointAt(offset);
if(!Character.isWhitespace(codePoint)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above: consider sticking a space between if and the opening parenthesis.

return false;
}
offset += Character.charCount(codePoint);
}
return true;
}

@Override
Expand Down
23 changes: 23 additions & 0 deletions hamcrest/src/test/java/org/hamcrest/text/IsBlankStringTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,28 @@ public final class IsBlankStringTest {
assertMatches(blankString(), " \t");
assertMatches(blankOrNullString(), " \t");
}

@Test public void
matchesAllCharactersConsideredWhitespaceByJavaLangCharacter() {
// See Javadocs for Character.isWhitespace
String[] consideredBlankByJavaLangCharacter = new String[] {
"\t",
"\n",
"\u000B",
"\f",
"\r",
"\u001C",
"\u001D",
"\u001E",
"\u001F"
};

for(String string : consideredBlankByJavaLangCharacter) {
assertTrue(string.isBlank());
assertMatches(blankString(), string);
assertMatches(blankOrNullString(), string);
}
}

@Test public void
doesNotMatchFilledString() {
Expand All @@ -52,4 +74,5 @@ public final class IsBlankStringTest {
assertMismatchDescription("was \"a\"", blankString(), "a");
assertMismatchDescription("was \"a\"", blankOrNullString(), "a");
}

}