Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
could you add to the PR description why are you adding this two calls to Charset instead of just passing
consoleEncoding
?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.
consoleEncoding might return null or what do you mean? I just moved the instruction out of the listener where it is unnecessarily reevaluated on each callback
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.
Previously the value returned by
builder.launch.getAttribute(DebugPlugin.ATTR_CONSOLE_ENCODING)
was passed totext.getBytes
as is (just casting toNonNull
).Now if the value is null we map it to a default charset. Is that an intentional change? If so I think it would be worth putting in the commit and PR messages.
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.
Because getBytes(String) would first lookup the CharSet object. If we obtain the Charset before and directly pass it to getBytes() we save that lookup on each listener event.
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.
Thanks for the explanation. To be equivalent as before, we should throw a NPE if
builder.launch.getAttribute(DebugPlugin.ATTR_CONSOLE_ENCODING)
, otherwise we are changing the behavior, are we not?I am fine changing the behaviour, but I think it should be added to the commit an PR messages.
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 actually think falling back to the default charset is more sensible than throwing an exception if the encoding is not overwritten in the launch config.
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.
Yes, me too.
Could you amend the commit message and add "If the console encoding was not specified in the launch config it now falls back to the JVM's default encoding instead of throwing an exception." there as well?
Then I am good