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.

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

Normalize breakingSpaces like regular whiteSpaces #3120

Merged
merged 5 commits into from
Oct 11, 2023

Conversation

maximedezette
Copy link
Contributor

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 ?

Copy link
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.

@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
@scordio scordio self-assigned this Jul 31, 2023
@scordio
Copy link
Member

scordio commented Aug 1, 2023

@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
Copy link
Contributor Author

@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
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
Contributor Author

@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 👍

@scordio scordio merged commit 4ba4718 into assertj:main Oct 11, 2023
11 checks passed
@scordio
Copy link
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

isEqualToNormalizingWhitespace doesn't normalise non breaking space characters
2 participants