Merge pull request #17656 from bernt-matthias/topic/lint-whitespace-text

tool linter: check for leaf nodes with unstripped text content

lib/galaxy/tool_util/linters/general.py

@@ -231,6 +231,23 @@ def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"):
                 )
 
 
+class TextSpaces(Linter):
+    @classmethod
+    def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"):
+        _, tool_node = _tool_xml_and_root(tool_source)
+        if not tool_node:
+            return
+        for node in tool_node.iter():
+            if len(node) > 0:
+                continue
+            if node.text and node.text != node.text.strip():
+                lint_ctx.warn(
+                    f"XML node '{node.tag}' has text with leading or trailing spaces ('{node.text}'!='{node.text.strip()}').",
+                    linter=cls.name(),
+                    node=node,
+                )
+
+
 class BioToolsValid(Linter):
     @classmethod
     def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"):

test/unit/tool_util/test_tool_linters.py

@@ -8,7 +8,6 @@
 from galaxy.tool_util.lint import (
     lint_tool_source_with,
     lint_tool_source_with_modules,
-    lint_xml_with,
     LintContext,
     Linter,
     XMLLintMessageLine,
@@ -30,7 +29,6 @@
 from galaxy.tool_util.parser.xml import XmlToolSource
 from galaxy.util import (
     ElementTree,
-    parse_xml,
     submodules,
 )
 from galaxy.util.unittest_utils import skip_if_site_down
@@ -122,6 +120,16 @@
 </tool>
 """
 
+GENERAL_TEXT_SPACES = """
+<tool name="valid name" id="valid_id" version="1.0+galaxy1" profile="21.09">
+    <xrefs>
+        <xref type="bio.tools">
+            bwa
+        </xref>
+    </xrefs>
+</tool>
+"""
+
 GENERAL_VALID_BIOTOOLS = """
 <tool name="valid name" id="valid_id" version="1.0+galaxy1" profile="23.0">
     <xrefs>
@@ -917,12 +925,14 @@
 """
 
 TOOL_WITH_COMMENTS = """
-<tool>
+<tool id="id" name="name" version="1" profile="22.01">
     <stdio>
-    <!-- This is a comment -->
+        <!-- This is a comment -->
+        <regex match="low space" source="both" level="warning" description="Low space on device" />
     </stdio>
     <outputs>
-    <!-- This is a comment -->
+        <!-- This is a comment -->
+        <data format="pdf" name="out_file1" />
     </outputs>
 </tool>
 """
@@ -950,15 +960,6 @@ def get_xml_tool_source(xml_string: str) -> XmlToolSource:
     return XmlToolSource(get_xml_tree(xml_string))
 
 
-def get_tool_xml_exact(xml_string: str):
-    """Returns the tool XML as it is, without stripping comments or anything else."""
-    with tempfile.NamedTemporaryFile(mode="w", suffix="tool.xml") as tmp:
-        tmp.write(xml_string)
-        tmp.flush()
-        tool_path = tmp.name
-        return parse_xml(tool_path, strip_whitespace=False, remove_comments=False)
-
-
 def run_lint_module(lint_ctx, lint_module, lint_target):
     lint_tool_source_with_modules(lint_ctx, lint_target, list({lint_module, xsd}))
 
@@ -1107,6 +1108,19 @@ def test_general_valid_new_profile_fmt(lint_ctx):
     assert not lint_ctx.error_messages
 
 
+def test_general_text_spaces(lint_ctx):
+    tool_source = get_xml_tool_source(GENERAL_TEXT_SPACES)
+    run_lint_module(lint_ctx, general, tool_source)
+    assert (
+        "XML node 'xref' has text with leading or trailing spaces ('\n            bwa\n        '!='bwa')"
+        in lint_ctx.warn_messages
+    )
+    assert not lint_ctx.info_messages
+    assert len(lint_ctx.valid_messages) == 4
+    assert len(lint_ctx.warn_messages) == 1
+    assert not lint_ctx.error_messages
+
+
 @skip_if_site_down("https://bio.tools/")
 def test_general_valid_biotools(lint_ctx):
     tool_source = get_xml_tool_source(GENERAL_VALID_BIOTOOLS)
@@ -1118,6 +1132,15 @@ def test_general_valid_biotools(lint_ctx):
     assert not lint_ctx.error_messages
 
 
+def test_general_text_spaces_comments(lint_ctx):
+    tool_source = get_xml_tool_source(TOOL_WITH_COMMENTS)
+    run_lint_module(lint_ctx, general, tool_source)
+    assert not lint_ctx.info_messages
+    assert len(lint_ctx.valid_messages) == 4
+    assert not lint_ctx.warn_messages
+    assert not lint_ctx.error_messages
+
+
 def test_general_valid_edam(lint_ctx):
     tool_source = get_xml_tool_source(GENERAL_VALID_EDAM)
     run_lint_module(lint_ctx, general, tool_source)
@@ -2118,13 +2141,6 @@ def test_linting_cwl_tool(lint_ctx):
     assert not lint_ctx.error_messages
 
 
-def test_xml_comments_are_ignored(lint_ctx: LintContext):
-    tool_xml = get_tool_xml_exact(TOOL_WITH_COMMENTS)
-    lint_xml_with(lint_ctx, tool_xml)
-    for lint_message in lint_ctx.message_list:
-        assert "Comment" not in lint_message.message
-
-
 def test_skip_by_name(lint_ctx):
     # add a linter class name to the skip list
     lint_ctx.skip_types = ["CitationsMissing"]
@@ -2152,7 +2168,7 @@ def test_skip_by_module(lint_ctx):
 def test_list_linters():
     linter_names = Linter.list_listers()
     # make sure to add/remove a test for new/removed linters if this number changes
-    assert len(linter_names) == 132
+    assert len(linter_names) == 133
     assert "Linter" not in linter_names
     # make sure that linters from all modules are available
     for prefix in [