pre-commit: Add xmlformat hook

[Holzhaus]

No description provided.

[Holzhaus]

Our controller XMLs are a wild mixture of tabs, spaces, weird formatting issues, etc. This makes sure that our XML files are always formatted properly.

[daschuer]

Does this format whole files?
I think due to our mixture of styles this will produce unreadable commits, this is even worse than the style inconsistency between different mappings, because a change is hidden into a big diff.

I think we should close this PR ... or disable this.
.. or can we enable it only for new files, or can we add all existing files to an ignore list or something?

[Holzhaus]

Does this format whole files?

Yes. The main issue with our XML files is broken indentation, which can't be fixed with a diff-based approach.

I think due to our mixture of styles this will produce unreadable commits, this is even worse than the style inconsistency between different mappings, because a change is hidden into a big diff.

To be honest, I hardly look at diffs of XML files anymore, because they are not really readable anyway, partly do to the nature of the XML syntax and partly due to mixing tabs and spaces within the same file.

What do we use XMLs for:

• Controller mappings
• Skins files
• Qt Designer files

The latter two are not affected by this PR because they use a different file extension some are autogenerated anyway. The two former ones are either long lists of MIDI assignments or skin markup, so I think it's not that bad if some commits produce bigger diffs IMHO. I think this is a small price to pay for eventually vastly more readable XMLs (and we also get the XML syntax check for free).

[daschuer]

The bad thing here is that you change a single line and the whole file is reformated producing a SINGLE commit hiding the real change

So we must not enforce it via a pre-commit hook.

I see two alternatives.

1. Drop this PR and don't care.
2. Reformat all XML in separate commits.

I will not spend my time to review 2, because there is no benefit. All XML code is already valid.

We can keep this hook disabled and ask mapping contributors to run it in demand if a reviewer is willing to review it. But it is unlikely and not necessary as you stated above.

We may also apply it on new files.

[Holzhaus]

I set this hook to manual, so it won't run automatically but we can still ask contributors to run it manually. I think this is ready to merge now.

Regarding 2): I'd be in favor of running it on all XML files in this repo in a single commit and then enable this hook afterwards.

I will not spend my time to review 2, because there is no benefit. All XML code is already valid.

In that case there is no need to review this commit because it only contains reformattings. The benefit is better readability through proper indentation and less headaches when editing these files. When editing the controller files for #3108, I had to disable all kinds of stuff in my editor, because some files use DOS line endings (or even a mixture of both UNIX and DOS line endings) and just touching the file already caused huge diffs. Same goes for missing newlines at EOF and stuff like that.

[daschuer]

Unfortunately the error message looks not that nice if there is an xml issue:

Traceback (most recent call last):
  File "tools/xmlformat.py", line 34, in <module>
    sys.exit(main())
  File "tools/xmlformat.py", line 19, in main
    dom = xml.dom.minidom.parseString(content)
  File "/usr/lib/python3.7/xml/dom/minidom.py", line 1968, in parseString
    return expatbuilder.parseString(string)
  File "/usr/lib/python3.7/xml/dom/expatbuilder.py", line 925, in parseString
    return builder.parseString(string)
  File "/usr/lib/python3.7/xml/dom/expatbuilder.py", line 223, in parseString
    parser.Parse(string, True)
xml.parsers.expat.ExpatError: not well-formed (invalid token): line 63, column 26

can we catch that without crashing?

[daschuer]

The hook fails to format the style block. The result is:

                                                       <TooltipId>track_title</TooltipId>
                                                        <Style>QLabel {
                              font: bold 13px sans-serif;
                              font-family: &quot;Open Sans&quot;;
                              background-color: transparent;
                              color: #191F24;
                              text-align: left;
                              padding-left: 1px;
                              padding-top: 4px;
                            }
                            </Style>
                                                        <Property>titleInfo</Property>

[daschuer]

The same happens for comments:

                                                        <Children>
                                                            <!--If you want the waveforms center to adjust when resizing in a collapsing widget
                              (e.g. to display spinning vinyl widget like in this skin) don't put the Waveform in a widget group,
                              and don't specify the Waveform's <Size>. So you would have:
                              Parent WidgetGroup
                                Waveform Widget (no Widgetgroup)
                                Child WidgetGroup for Spinny
                                  Spinny Widget
                              -->
                                                            <!--
                              **********************************************
                              Visual- Waveform
                              **********************************************
                              -->
                                                            <WidgetGroup>

[daschuer]

Did you consider to use two space indent? This would make the style consistent with QT's UI files.

[ronso0]

yes, please let's stick to 2-space indent. Just one isn't enough to navigate skin templates IMO.

[Holzhaus]

yes, please let's stick to 2-space indent. Just one isn't enough to navigate skin templates IMO.

This is currently using 4 spaces, but I can reduce it to 2 if you prefer.

[ronso0]

We have 2 spaces in all skin files I worked on so far and this is very confortable to work with (except Shade where indentation is ... "variable" ;).
Given the fact that skin files can go down 15 levels and more I'd rather stick to 2 spaces as compromise.

[Holzhaus]

I think I worked around the issues, please retest.

[daschuer]

There is still one an issue with Variables:

-                                    <ObjectName>Waveform<Variable name="channum"/></ObjectName>
+                                    <ObjectName>
+                                                                            Waveform
+                                      <Variable name="channum"/>
+                                    </ObjectName>

[daschuer]

The rest works already nice. Thank you.

[ronso0]

How do I activate the new hook?
The docu https://github.com/mixxxdj/mixxx/wiki/Using-Git#set-up-automatic-code-checking does not mention this (and I don't find the PR where I probably asked this earlier)
The pre-commit docu is not helpful either. pre-commit run xmlformat says No hook with this id

[Holzhaus]

There is still one an issue with Variables:

-                                    <ObjectName>Waveform<Variable name="channum"/></ObjectName>
+                                    <ObjectName>
+                                                                            Waveform
+                                      <Variable name="channum"/>
+                                    </ObjectName>

I rewrote that part to always remove whitespace around <Variable> tags. Please check again.

How do I activate the new hook?
The docu https://github.com/mixxxdj/mixxx/wiki/Using-Git#set-up-automatic-code-checking does not mention this (and I don't find the PR where I probably asked this earlier)
The pre-commit docu is not helpful either. pre-commit run xmlformat says No hook with this id

$ pre-commit run xmlformat --hook-stage manual --files res/skins/Shade/*.xml

[ronso0]

diff --git a/res/skins/LateNight/mixer.xml b/res/skins/LateNight/mixer.xml

@@ -1,6 +1,8 @@
+<?xml version="1.0" ?>
 <Template>
-  <SetVariable name="BtnType"><Variable name="TopRegion_BtnType"/></SetVariable>
-
+  <SetVariable name="BtnType">
+    <Variable name="TopRegion_BtnType"/>
+  </SetVariable>
   <WidgetGroup>
     <SizePolicy>max,me</SizePolicy>
     <Layout>horizontal</Layout>
@@ -9,35 +11,32 @@
       <BindProperty>visible</BindProperty>
     </Connection>
     <Children>
-
       <WidgetGroup>
         <ObjectName>MixerContainer</ObjectName>
         <SizePolicy>min,me</SizePolicy>
         <Layout>horizontal</Layout>
         <Children>
-
           <Template src="skin:/mixer/mixer_decks.xml"/>
-
           <Template src="skin:/mixer/mixer_master_headphone.xml"/>
-
         </Children>
-      </WidgetGroup><!-- MixerContainer -->
-
+      </WidgetGroup>
+      <!-- MixerContainer -->
     </Children>
-  </WidgetGroup><!-- show_mixer -->
-
+  </WidgetGroup>
+  <!-- show_mixer -->
   <WidgetGroup>
     <SizePolicy>max,me</SizePolicy>
     <Layout>horizontal</Layout>
     <Connection>
       <ConfigKey persist="true">[Master],show_mixer</ConfigKey>
-      <Transform><Not/></Transform>
+      <Transform>
+        <Not/>
+      </Transform>
       <BindProperty>visible</BindProperty>
     </Connection>
     <Children>
-
       <WidgetGroup>
-        <ObjectName></ObjectName>
+        <ObjectName/>
         <SizePolicy>min,me</SizePolicy>
         <Layout>vertical</Layout>
         <Connection>
@@ -45,14 +44,12 @@
           <BindProperty>visible</BindProperty>
         </Connection>
         <Children>
-
           <WidgetGroup>
             <!--  width set in qss  -->
             <ObjectName>CompactDecksCenterSpacer</ObjectName>
           </WidgetGroup>
-
           <WidgetGroup>
-            <ObjectName></ObjectName>
+            <ObjectName/>
             <SizePolicy>min,me</SizePolicy>
             <Layout>vertical</Layout>
             <Connection>
@@ -63,10 +60,9 @@
               <Template src="skin:/mixer/vumeters_compact.xml"/>
             </Children>
           </WidgetGroup>
-
         </Children>
-      </WidgetGroup><!-- !show_mixer -->
-
+      </WidgetGroup>
+      <!-- !show_mixer -->
     </Children>
   </WidgetGroup>
 </Template>

Besides the format repair this hook unfortunately also alters some intended formats useful for navigation

• removes blank lines separators
• moves comments to new lines, like repeated ObjectName at the closing tag of a large widget
• splits one-line <Transform> and <SetVariable ...> nodes onto separate lines
• empty nodes are condensed (<ObjectName/>), > we may leave them untouched or remove them

Considering this hook will mostly be run manually to fix remarkably malformed files, it may be hard to pick only desired fixes with git add -p afterwards, especially when a lot of space/tab indentations are fixed.
I suggest we agree on some style rules, even though I don't find any bad examples right now in our repo.
(referring to skin templates only)

[Holzhaus]

• removes blank lines separators

I'll have a look.

• moves comments to new lines, like repeated ObjectName at the closing tag of a large widget

I actually think that's okay. Just put it in the line above the closing tag.

• splits one-line <Transform> and <SetVariable ...> nodes onto separate lines

For <Transform> I don't see a problem. I'll look into <SetVariable>.

• empty nodes are condensed (<ObjectName/>), > we may leave them untouched or remove them

This is done automatically as per the XML standard. I can't remove all closed nodes (otherwise it would also remove <Not /> inside <Transform>. I think the current behavior is okay. Just remove them manually, I don't know why someone should add empty objectname tags in the first place.

[ronso0]

• splits one-line <Transform> and <SetVariable ...> nodes onto separate lines

For <Transform> I don't see a problem. I'll look into <SetVariable>.

I just wish to keep the <Connection> nodes compact , that's all. Mostly it's just <Not/ and <IsEqual>n</IsEqual> which doesn't require separate lines for each command.

This is done automatically as per the XML standard. I can't remove all closed nodes (otherwise it would also remove inside . I think the current behavior is okay.

👍

[Holzhaus]

I dropped the custom script since preserving blank lines was not possible and I didn't fine a way to work around that. Instead, this now uses the xmlformatter python package with some custom patches.

Downside: the CSS formatting in the stylesheet might not be indented correctly, but I think we should treat that as legacy anyway and move that stuff into the QSS files if possible.

Please test again.

[github-actions]

This PR is marked as stale because it has been open 90 days with no activity.

[Holzhaus]

@ronso0 Any comments? Is something like this desired?

[Holzhaus]

ping

[ronso0]

sry, this was not on the top of my list and it requires some time to evaluate. I'll check this tonight or tomorrow, okay?

[Holzhaus]

sry, this was not on the top of my list and it requires some time to evaluate. I'll check this tonight or tomorrow, okay?

Don't worry, if you have stuff to do for release it's fine to prioritize that. Just wanted to make sure this isn't forgotten ;-)

[ronso0]

🔖 Saved ; )

[Holzhaus]

Btw, I also experimented with a CSS formatter hook for the *.qss files, but unfortunately csscomb is buggy and installing a custom version from Git does not work. So that is on hold for now.

[github-actions]

This PR is marked as stale because it has been open 90 days with no activity.

[acolombier]

@Holzhaus are you still interesting in completing this? Personally, I'm keen to enforce some linting, even if we loose some current style (e.g the compact form is IMO not a big deal, as long as it remains consistent).

Other point for experience with introducing this kind of linting enforcement on already existing code base is the amount of noise it generates as soon as somebody make a small change, as it will trigger the hook. Usually, my recommendation is to ask contributor to make two commits (e.g one chore: lint the XXX mapping defs and the second one for the actual change) but this may have limited feasibility with less experimented contributors, which may be more common on mapping PRs.

How would you feel about running a --all-files commits and lint all the mappings? I appreciate this would make one hell of a big commit, but if we are confident about our linter (are we?), there shouldn't be any risk associated with it.

[daschuer]

I set this hook to manual, so it won't run automatically but we can still ask contributors to run it manually. I think this is ready to merge now.

I prefere that.

I had recently to deal with many conflicts because of the introduced CMake reformating. The conflict resolution are normally not well tested. Here we are touching many more files and we have 55 PRs open changing them. So please let's not put this hassle on us here as well.
Have an automated way to enforce a certain style is very welcome.

[acolombier]

Yeah, that a good point. Is this ready for merge once the conflict is resolved then?

[acolombier]

Marking as draft for now, but feel free to make ready once you've had time to solve the conflict!