Converted AssertJ assetions into more redable/idiomatic (e.g. instead of assertThat(collection.size()).equals(5) -> assertThat(collection).hasSize(5)) - besides shorter and more readable code it also produces more appropriate error messages.

Comment From: sbrannen

Hi @krzyk,

Thanks for the PR!

This is a task that I was planning on putting "up for grabs" via our status: ideal-for-contribution label. So it's nice to see that you've taken the initiative on your own.

Out of curiosity, how did you perform these refactorings?

Do you have a tool that assists you, or did you create your own tool (or regular expressions) to achieve the refactorings?

For future reference, if you split up changes like this into multiple commits (either by module or perhaps preferably by type of refactoring) that will make it easier for core maintainers to process the PR.

Cheers,

Sam

Comment From: krzyk

@sbrannen Most of those were suggested by IntelliJ IDEA plugin https://plugins.jetbrains.com/plugin/12195-concise-assertj-optimizing-nitpicker-cajon- . It is not perfect (some suggestions do not compile, some fail during runtime, AFAIR it is related to generics), but it was a real time saver.

Others I found during checking the tests manually and debugging issues from the plugin.

Comment From: sbrannen

Thanks for the feedback.

That Cajon plugin for IDEA looks quite powerful.

The Spring team could have really used that a while back when we migrated from JUnit assertions to AssertJ (which coincidentally resulted in the non-idiomatic usage of AssertJ APIs).

/cc @philwebb

Comment From: philwebb

Interesting tool, thanks for /cc'ing me @sbrannen.

Comment From: simonbasle

You can fix these now or hold off until we've reviewed the full set of changes.

if you change anything @krzyk please do so with incremental commits and not a rebase / force-push 🙇

Comment From: krzyk

.isEqualTo(x) on array length or collection sizes where hasSize(x)

Just a note that in some places that is not possible even if it looks like it should be - of course there are probably places which I missed.

Comment From: krzyk

if you change anything @krzyk please do so with incremental commits and not a rebase / force-push bow

Sure, at first I wasn't sure what's your take on the amount of commits in a single PR, some project prefer to have a single one, others like to squash them in a single one.

So I'm holding off on rebasing until you finish reviewing, I think this will be easier.

Again sorry for such big PR - it won't happen again :)

Comment From: krzyk

@simonbasle I wonder (if you looked already at spring-context) maybe it would be better if I revert my changes to the other subprojects (in this PR) and create few other PRs with changes in those?

Comment From: simonbasle

@simonbasle I wonder (if you looked already at spring-context) maybe it would be better if I revert my changes to the other subprojects (in this PR) and create few other PRs with changes in those?

could be interesting yeah, if you manage to split the remainder into PRs of comparable sizes. I think the most changes are in the web module, which would definitely make sense as its own PR, for a start

Sure, at first I wasn't sure what's your take on the amount of commits in a single PR, some project prefer to have a single one, others like to squash them in a single one.

That's personal preference but I like the commits to be incremental during the review phase (so I don't loose track of what I've reviewed initially and what comes from review process). then at the end I'll squash and merge, to have a single meaningful commit with a detailed commit message.

Again sorry for such big PR - it won't happen again :)

No worries :)

Comment From: simonbasle

@krzyk I think I'll be able to review everything today or tomorrow, so no need to split the PR for now (but something to be kept in mind for later ones 😉)

Comment From: krzyk

@simonbasle I updated the code and merged upstream changes.