gnodet-bot commented on code in PR #1214:
URL:
https://github.com/apache/maven-plugin-tools/pull/1214#discussion_r4301918791
##########
maven-plugin-tools-generators/src/main/java/org/apache/maven/tools/plugin/generator/HtmlToPlainTextConverter.java:
##########
@@ -78,8 +91,20 @@ public void head(Node node, int depth) {
// hit when all of the node's children (if any) have been visited
public void tail(Node node, int depth) {
String name = node.nodeName();
- if (StringUtil.in(name, "br", "dd", "dt", "p", "h1", "h2", "h3",
"h4", "h5")) {
- accum.append("\n");
+ boolean preformattedEndsWithNewline = false;
+ if (name.equals("pre")) {
+ preformatted--;
+ preformattedEndsWithNewline = accum.length() > 0 &&
accum.charAt(accum.length() - 1) == '\n';
+ accum.append(PRE_END);
+ }
+ if (name.equals("pre")) {
+ if (!preformattedEndsWithNewline) {
+ accum.append('\n');
+ }
Review Comment:
⚠️ **Redundant second `if (name.equals("pre"))` block** — the two
consecutive `if` blocks can be merged. After the first block appends `PRE_END`,
`name` is still `"pre"`, so the second check is always true when the first was
true. Merge them into a single block:
```suggestion
if (name.equals("pre")) {
preformatted--;
preformattedEndsWithNewline = accum.length() > 0 &&
accum.charAt(accum.length() - 1) == '\n';
accum.append(PRE_END);
if (!preformattedEndsWithNewline) {
accum.append('\n');
}
```
##########
maven-plugin-tools-generators/src/test/java/org/apache/maven/tools/plugin/generator/HtmlToPlainTextConverterTest.java:
##########
@@ -85,6 +85,15 @@ void explicitNewline() {
converter.convert(test));
}
+ @Test
+ void preformattedTextPreservesWhitespace() {
+ String test =
+ "For example:<pre>\n <binary>\n
<url>https://example.test/plugin.exe</url>\n
</binary>\n</pre>";
+ assertEquals(
+ "For example:\n <binary>\n
<url>https://example.test/plugin.exe</url>\n </binary>\n",
Review Comment:
💡 **Consider adding edge-case tests** for robustness:
1. `<pre>` content that does **not** end with a newline (tests the
`preformattedEndsWithNewline` branch)
2. Multiple `<pre>` blocks in the same input
3. Nested `<pre>` handling (if expected to be unsupported, a comment saying
so would help)
The current test only covers the trailing-newline path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
