Skip to content

Normalize breakingSpaces like regular whiteSpaces#3120

Merged
scordio merged 5 commits intoassertj:mainfrom
maximedezette:fix-whitespace-normalization
Oct 11, 2023
Merged

Normalize breakingSpaces like regular whiteSpaces#3120
scordio merged 5 commits intoassertj:mainfrom
maximedezette:fix-whitespace-normalization

Conversation

@maximedezette
Copy link
Copy Markdown
Contributor

@maximedezette maximedezette commented Jul 24, 2023

Check List:

Following the contributing guidelines will make it easier for us to review and accept your PR.

Note

I think that the breaking-space management could be improved by using a public constant so that we don't have to update all the tests if some of the breaking space codes change over time. It would look like this:

    Stream<Arguments> breakingSpaces = Arrays.stream(Strings.BREAKING_SPACES)
      .map(breakingSpace -> Arguments.of("my" + breakingSpace + "foo bar", "my foo bar"));
    Stream<Arguments> regularSpaces = Stream.of(Arguments.of("my   foo bar", "my foo bar"),
      Arguments.of("  my foo bar  ", "my foo bar"),
      Arguments.of(" my\tfoo bar ", " my foo bar"),
      Arguments.of(" my foo    bar ", "my foo bar"),
      Arguments.of(" my foo    bar ", "  my foo bar   "),
      Arguments.of("       ", " "),
      Arguments.of(" my\tfoo bar ", new String(arrayOf(' ', 'm', 'y', ' ', 'f', 'o', 'o', ' ', 'b', 'a', 'r'))),
      Arguments.of(" my\tfoo bar ", " my\tfoo bar "),   // same
      Arguments.of(null, null),   // null
      Arguments.of(" \t \t", " "),
      Arguments.of(" abc", "abc ")
    );
    return Stream.concat(regularSpaces, breakingSpaces);

Instead of having to update all the tests:

image

What do you think ?

maximedezette
maximedezette commented Jul 24, 2023
View reviewed changes
Comment thread assertj-core/src/main/java/org/assertj/core/internal/Strings.java Outdated
maximedezette
maximedezette commented Jul 24, 2023
View reviewed changes
Comment thread assertj-core/src/main/java/org/assertj/core/internal/Strings.java Outdated
@maximedezette maximedezette mentioned this pull request Jul 24, 2023
Closed
@scordio scordio added this to the 3.25.0 milestone Jul 25, 2023
scordio
scordio requested changes Jul 25, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@scordio scordio left a comment

Choose a reason for hiding this comment

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

Thank you, @maximedezette! I've put some comments.

Could you please also enhance the Javadoc as mentioned in #3113 (comment)?

Lastly, the build is failing due to formatting issues. You can run ./mvnw spotless:apply to make sure that all changes adhere to the expected code style.

Comment thread assertj-core/src/main/java/org/assertj/core/internal/Strings.java Outdated
Comment thread assertj-core/src/main/java/org/assertj/core/internal/Strings.java Outdated
Comment thread ...t/java/org/assertj/core/internal/strings/Strings_assertEqualsNormalizingWhitespace_Test.java Outdated
@maximedezette maximedezette force-pushed the fix-whitespace-normalization branch 2 times, most recently from 1b874bb to 5670feb Compare July 29, 2023 13:21
@maximedezette maximedezette marked this pull request as draft July 29, 2023 13:39
@maximedezette maximedezette force-pushed the fix-whitespace-normalization branch 2 times, most recently from 229e4f1 to 0d5edf8 Compare July 29, 2023 14:22
@maximedezette maximedezette marked this pull request as ready for review July 29, 2023 15:19
@maximedezette maximedezette requested a review from scordio July 29, 2023 15:19
@scordio scordio self-assigned this Jul 31, 2023
@scordio
Copy link
Copy Markdown
Member

scordio commented Aug 1, 2023 •
edited
Loading

@maximedezette This PR has now several merge conflicts but you don't need to worry about them: I've taken the opportunity to do some refactoring in the surrounding area to better evaluate if we should also change other assertions relying on whitespaces.

Once I'm done with the refactoring, I'll cherry-pick your changes maintaining you as the author.

@maximedezette maximedezette force-pushed the fix-whitespace-normalization branch from 0d5edf8 to 86fd497 Compare August 2, 2023 18:56
@maximedezette
Copy link
Copy Markdown
Contributor Author

maximedezette commented Aug 2, 2023

@scordio I have missed your comment mentioning the cherry-pick and I've done the rebase 😅

I have thrown away a bit of code about the refactoring of non-breaking spaces tests but I'll do an other PR if I feel that it could improve the tests after your refactoring 👍

@scordio
Copy link
Copy Markdown
Member

scordio commented Aug 25, 2023

@maximedezette sorry for the slow feedback!

I just refreshed your branch and made some cosmetic changes.

Also, I noticed that this change will also affect isNotEqualToNormalizingWhitespace and isEqualToNormalizingPunctuationAndWhitespace. I already aligned the Javadoc but we should also have tests for those assertions.

Would you like to add the missing tests? No problem if you don't have time, I can take care of them next week.

@maximedezette
Copy link
Copy Markdown
Contributor Author

maximedezette commented Sep 2, 2023

@scordio I don't have time at the moment but if it can wait a few days, I should be able to add those tests this week 👍

@maximedezette maximedezette force-pushed the fix-whitespace-normalization branch from bb71108 to 1b97713 Compare October 1, 2023 12:37
@scordio scordio merged commit 4ba4718 into assertj:main Oct 11, 2023
@scordio
Copy link
Copy Markdown
Member

scordio commented Oct 11, 2023

Thanks for your first contribution, @maximedezette!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@scordio scordio scordio approved these changes

Assignees

@scordio scordio

Labels

None yet

Projects

None yet

Milestone

3.25.0

Development

Successfully merging this pull request may close these issues.

isEqualToNormalizingWhitespace doesn't normalise non breaking space characters

2 participants

@maximedezette @scordio