Re: [PR] fix: preserve preformatted help descriptions [maven-plugin-tools]

["gnodet-bot (via GitHub)" <gi...@apache.org>]

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  &lt;binary&gt;\n    &lt;url&gt;https://example.test/plugin.exe&lt;/url&gt;\n  &lt;/binary&gt;\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: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org