Conversation
Signed-off-by: Pavel Jareš <Pavel.Jares@broadcom.com>
Signed-off-by: Pavel Jareš <Pavel.Jares@broadcom.com>
|
Description of the PR? Maybe you can link to the upstream fix? |
iansergeant42
left a comment
There was a problem hiding this comment.
Comment left as I can't request changes
| @Override | ||
| public SseEventBuilderFixedImpl id(String id) { | ||
| checkEvent(id); | ||
| append("id:").append(id).append('\n'); |
There was a problem hiding this comment.
Literal \n is used a lot. Can it be a constant?
There was a problem hiding this comment.
It is following the original implementation to be easier to maintain.
| import java.util.Arrays; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.*; |
There was a problem hiding this comment.
Idea has removed imports again. Concern as with the previous PR.
|
|
||
| private static void checkEvent(String content) { | ||
| Assert.isTrue(content.indexOf('\n') == -1 && content.indexOf('\r') == -1, | ||
| "illegal character '\\n' or '\\r' in event content"); |
There was a problem hiding this comment.
This is inconsistent with what is on lines 174 and 175. Can we please make them consistent, and use a constant?
There was a problem hiding this comment.
It is following the original implementation to be easier to maintain.
| char c = input.charAt(i); | ||
| if (c == '\r') { | ||
| if (i + 1 < length && input.charAt(i + 1) == '\n') { | ||
| i++; |
There was a problem hiding this comment.
Sonar doesn't like the incrementation of the counter within the loop.
There was a problem hiding this comment.
It is following the original implementation to be easier to maintain.
| @Mock | ||
| private SseEmitter emitter; | ||
|
|
||
| @Test |
There was a problem hiding this comment.
I don't see any benefit. If the exception is thrown it is good to have the stack trace.
| java.util.function.Consumer<ServerSentEvent<String>> consumer = new ServerSentEventProxyHandler(null, null) | ||
| .consumer(emitter); | ||
|
|
||
| assertThrows(IllegalArgumentException.class, () -> consumer.accept(event)); |
There was a problem hiding this comment.
Can we add the expected message for clarity?
| java.util.function.Consumer<ServerSentEvent<String>> consumer = new ServerSentEventProxyHandler(null, null) | ||
| .consumer(emitter); | ||
|
|
||
| assertThrows(IllegalArgumentException.class, () -> consumer.accept(event)); |
Signed-off-by: Pavel Jareš <Pavel.Jares@broadcom.com>
|


Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Linked to # (issue)
Part of the # (epic)
Type of change
Please delete options that are not relevant.
Checklist:
For more details about how should the code look like read the Contributing guideline