-
Notifications
You must be signed in to change notification settings - Fork 889
MessageBuilder: fix typo #688
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
base: master
Are you sure you want to change the base?
Conversation
| protected void addStanzaSpecificAttributes(ToStringUtil.Builder builder) { | ||
| builder.addValue("type", type) | ||
| ; | ||
| builder.addValue("type", type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's an unrelated change in this commit. Also commit subject could be "[core] Fix typo in MessageBuilder"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Such kind of change (obvious code style cleanup) is too small for a separate commit: too many of them will distract. I had to mention it in a commit message, but again it will be longer to describe than see.
Flowdalic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the point in switching to String concatenation over StringBuilder. Yes, javac will probably transform String concatenation to StringBuilder and yes, one could argue it's slightly more readable. But am I not convinced that it's worth to change.
In general, please make sure that you don't conflate multiple changes within the same commit (or even, within the same PR). This makes it easier to review stuff, keeps the history clean and increases the chances for undisputed changes to land.
| controlLanguages.add(lang3); | ||
| controlLanguages.removeAll(languages); | ||
| assertTrue(controlLanguages.size() == 0); | ||
| assertTrue(controlLanguages.isEmpty()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is ok
| .addBody("es", "test") | ||
| .build(); | ||
| assertTrue(message.getBodies().size() == 1); | ||
| assertEquals(1, message.getBodies().size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those changes are ok
| var message = assertInstanceOf(Message.class, stanzas.get(0)); | ||
| // In this case, no xml:lang on the stream or the message and no explicit language argument provided to | ||
| // getBody(), it is anbiguous which body we should return. | ||
| // getBody(), it is ambiguous which body we should return. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is also ok.
The javac will use concatenation with |
Also cleanup MessageTest