-
Notifications
You must be signed in to change notification settings - Fork 41.7k
Remove carriage returns in TypeUtils.getJavaDoc() #13779
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
Conversation
|
On a second thought both line feed ('\n') and carriage return ('\r') should be removed, not system-dependent line separator. I'll update shortly. |
dd56e22 to
f1997cf
Compare
|
I've updated to remove carriage returns in addition to line feeds. |
snicoll
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.
Thanks again for the PR! I've added a couple of comments
| if (javadoc != null) { | ||
| javadoc = javadoc.replaceAll("\\n", ""); | ||
| javadoc = javadoc.trim(); | ||
| javadoc = javadoc.replaceAll("[\r\n]+", "").trim(); |
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.
Can we do two passes instead (\r\n then \n)? Technically speaking that would replace \r
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.
@snicoll I thought removing "\r" in Javadoc, especially as description for application properties is safe. Am I missing something?
| ? this.env.getElementUtils().getDocComment(element) : null); | ||
| if (javadoc != null) { | ||
| javadoc = javadoc.replaceAll("\\n", ""); | ||
| javadoc = javadoc.trim(); |
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 still would like surrounding spaces to be removed. Do you disagree with that?
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.
@snicoll No, I just collapsed the two statements into one. I thought it's less verbose than before.
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 missed that 😓
* pr/13779: Remove carriage returns in TypeUtils.getJavaDoc()
|
Thanks a lot for the feedback @izeye |
This PR changes to use system-dependent line separator in
TypeUtils.getJavaDoc().