XML Pretty-print should be an editorial concern not enforced during the build

[adamretter]

Some of the changes suggested by Pretty-print are not very good.
Therefore I think it is better to have the option to run Pretty-print, but not to enforce it as part of the build. It was previously only enforced on code-listing and not articles, however the suggestions it was making for the code-listings would actually made their formatting worse; therefore it is best to leave it to the discretion of the Editor.

[duncdrum]

not really good is subjective we can configure suggestions we don't like.
leaving it to the editor means that with 20 PRs we get 20 different indentations and linting preferences increasing the maintenance burden when reviewing PRs. We're also decreasing the readability of the docs as a whole, since listings on different pages have different appearances.

[adamretter]

@duncdrum The problem is the not using the pretty-printing. The problem is automatically using the pretty-printing.

[duncdrum]

I don't see how not automating it, will benefit the project, other than making it more difficult to maintain, or decreasing consistency. If the existing formatting settings cause issues with listings, editors are free to adjust these and include that as part of their PR so we can review and discuss the changes properly.

It's only when we started doing this as part of the build, that side scrolling to actually see the contents of code-listings became a thing of the past. I m afraid that by disabling it, we're moving backwards. We should enforce a central style that also covers code listings, i don't care how we do it. As long as we do it, your descriptions reads to me that you don't want us to do that; I disagree.

[line-o]

My position is clear and in line with @duncdrum , auto formatting everything (code, articles, ...) is an important part to a maintainable project.

[line-o]

Would you be able to present an example that illustrates the necessity to remove this check?

[adamretter]

@line-o Sure. Before this PR run mvn clean package and you will see two issues:

1. The pretty-printing modifies the files in src/ which leads to Git staged issues.
2. Do a git diff and look at the changes, the formatting after the pretty-printing is considerably worse. Especially for the Maven Examples where that syntax would never be formatted in that way in any project.

[line-o]

Before this PR run mvn clean package and you will see two issues:

I just did this with b071189
After man clean package, git diff shows

• some untracked files (some Javascript sources, these were gone after git clean -f ; see [BUG] mvn package should not modify files in the src folder #633 )
• no changed file

[adamretter]

To reproduce the issue, simply run:

git clone https://github.com/eXist-db/documentation.git
mvn clean verify

You then have a bunch of suggestions via the auto-formatting which are unstaged as far as Git is concerned:

❯ git status
On branch master
Your branch is up to date with 'origin/master'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   src/main/xar-resources/data/lucene/listings/listing-311.xml
	modified:   src/main/xar-resources/data/xqsuite/listings/custom-assertion-result.xml

And... these suggestions are not great as you can see below in the diff, they introduce a great deal of white-space that is very unnatural:

diff --git a/src/main/xar-resources/data/lucene/listings/listing-311.xml b/src/main/xar-resources/data/lucene/listings/listing-311.xml
index 3d1ba57..7f967b1 100644
--- a/src/main/xar-resources/data/lucene/listings/listing-311.xml
+++ b/src/main/xar-resources/data/lucene/listings/listing-311.xml
@@ -1,8 +1,14 @@
 <analyzer id="my-custom-analyzer" class="tld.org.CustomAnalyzer">
-    <param name="minimumTermLength" type="int" value="2" />
-    <param name="punctuationDictionary" type="char[]">
-        <value>'</value>
-        <value>-</value>
-        <value>’</value>
-    </param>
+  <param name="minimumTermLength" type="int" value="2"/>
+  <param name="punctuationDictionary" type="char[]">
+    <value>
+      '
+    </value>
+    <value>
+      -
+    </value>
+    <value>
+      ’
+    </value>
+  </param>
 </analyzer>
diff --git a/src/main/xar-resources/data/xqsuite/listings/custom-assertion-result.xml b/src/main/xar-resources/data/xqsuite/listings/custom-assertion-result.xml
index f9b5ff2..c59cf9a 100644
--- a/src/main/xar-resources/data/xqsuite/listings/custom-assertion-result.xml
+++ b/src/main/xar-resources/data/xqsuite/listings/custom-assertion-result.xml
@@ -1,18 +1,23 @@
 <testsuites>
-    <testsuite package="http://exist-db.org/xquery/test/xqsuite" timestamp="2022-04-12T14:24:15.72+02:00"
-        tests="4" failures="3" errors="0" pending="0" time="PT0.008S">
-        <testcase name="test-missing-key" class="t:test-missing-key">
-            <failure message="Key 'b' is missing" type="map-assertion"/>
-            <output>map{"a":1,"c":4}</output>
-        </testcase>
-        <testcase name="test-passing" class="t:test-passing"/>
-        <testcase name="test-wrong-type" class="t:test-wrong-type">
-            <failure message="Type mismatch" type="map-assertion"/>
-            <output>[1,2]</output>
-        </testcase>
-        <testcase name="test-wrong-value" class="t:test-wrong-value">
-            <failure message="Value mismatch for key 'b'" type="map-assertion"/>
-            <output>map{"a":1,"b":3}</output>
-        </testcase>
-    </testsuite>
+  <testsuite package="http://exist-db.org/xquery/test/xqsuite" timestamp="2022-04-12T14:24:15.72+02:00" tests="4" failures="3" errors="0" pending="0" time="PT0.008S">
+    <testcase name="test-missing-key" class="t:test-missing-key">
+      <failure message="Key 'b' is missing" type="map-assertion"/>
+      <output>
+        map{"a":1,"c":4}
+      </output>
+    </testcase>
+    <testcase name="test-passing" class="t:test-passing"/>
+    <testcase name="test-wrong-type" class="t:test-wrong-type">
+      <failure message="Type mismatch" type="map-assertion"/>
+      <output>
+        [1,2]
+      </output>
+    </testcase>
+    <testcase name="test-wrong-value" class="t:test-wrong-value">
+      <failure message="Value mismatch for key 'b'" type="map-assertion"/>
+      <output>
+        map{"a":1,"b":3}
+      </output>
+    </testcase>
+  </testsuite>
 </testsuites>

[joewiz]

I saw this myself today when running the tests described in the README before I updated #784.

[adamretter]

@joewiz I think you also reported the same problem (as we have both seen) in a previous issue here too - #742 (comment)

[joewiz]

@adamretter Indeed!!