Merge pull request #18091 from bernt-matthias/skip-name-restore

[24.0] Tool linters: allow to skip by old linter names (by allowing to skip linter modules)

lib/galaxy/tool_util/lint.py

@@ -212,11 +212,19 @@ def found_errors(self) -> bool:
     def found_warns(self) -> bool:
         return len(self.warn_messages) > 0
 
-    def lint(self, name: str, lint_func: Callable[[LintTargetType, "LintContext"], None], lint_target: LintTargetType):
+    def lint(
+        self,
+        name: str,
+        lint_func: Callable[[LintTargetType, "LintContext"], None],
+        lint_target: LintTargetType,
+        module_name: Optional[str] = None,
+    ):
         if name.startswith("lint_"):
             name = name[len("lint_") :]
         if name in self.skip_types:
             return
+        if module_name and module_name in self.skip_types:
+            return
 
         if self.level < LintLevel.SILENT:
             # this is a relict from the past where the lint context
@@ -355,6 +363,7 @@ def lint_tool_source_with_modules(lint_context: LintContext, tool_source, linter
     tool_type = tool_source.parse_tool_type() or "default"
 
     for module in linter_modules:
+        module_name = module.__name__.split(".")[-1]
         lint_tool_types = getattr(module, "lint_tool_types", ["default", "manage_data"])
         if not ("*" in lint_tool_types or tool_type in lint_tool_types):
             continue
@@ -374,7 +383,7 @@ def lint_tool_source_with_modules(lint_context: LintContext, tool_source, linter
                 else:
                     lint_context.lint(name, value, tool_source)
             elif inspect.isclass(value) and issubclass(value, Linter) and not inspect.isabstract(value):
-                lint_context.lint(name, value.lint, tool_source)
+                lint_context.lint(name, value.lint, tool_source, module_name=module_name)
     return lint_context
 
 

test/unit/tool_util/test_tool_linters.py

@@ -1,8 +1,10 @@
+import inspect
 import os
 import tempfile
 
 import pytest
 
+import galaxy.tool_util.linters
 from galaxy.tool_util.lint import (
     lint_tool_source_with,
     lint_tool_source_with_modules,
@@ -18,7 +20,7 @@
     general,
     help,
     inputs,
-    outputs,
+    output,
     stdio,
     tests,
     xml_order,
@@ -29,6 +31,7 @@
 from galaxy.util import (
     ElementTree,
     parse_xml,
+    submodules,
 )
 from galaxy.util.xml_macros import load_with_references
 
@@ -1508,7 +1511,7 @@ def test_inputs_repeats(lint_ctx):
 
 def test_outputs_missing(lint_ctx):
     tool_source = get_xml_tool_source(OUTPUTS_MISSING)
-    run_lint_module(lint_ctx, outputs, tool_source)
+    run_lint_module(lint_ctx, output, tool_source)
     assert "Tool contains no outputs section, most tools should produce outputs." in lint_ctx.warn_messages
     assert not lint_ctx.info_messages
     assert not lint_ctx.valid_messages
@@ -1518,7 +1521,7 @@ def test_outputs_missing(lint_ctx):
 
 def test_outputs_multiple(lint_ctx):
     tool_source = get_xml_tool_source(OUTPUTS_MULTIPLE)
-    run_lint_module(lint_ctx, outputs, tool_source)
+    run_lint_module(lint_ctx, output, tool_source)
     assert "0 outputs found." in lint_ctx.info_messages
     assert "Invalid XML: Element 'outputs': This element is not expected." in lint_ctx.error_messages
     assert len(lint_ctx.info_messages) == 1
@@ -1535,7 +1538,7 @@ def test_outputs_unknown_tag(lint_ctx):
     probably be avoided.
     """
     tool_source = get_xml_tool_source(OUTPUTS_UNKNOWN_TAG)
-    lint_tool_source_with_modules(lint_ctx, tool_source, [outputs, xsd])
+    lint_tool_source_with_modules(lint_ctx, tool_source, [output, xsd])
     assert "0 outputs found." in lint_ctx.info_messages
     assert "Avoid the use of 'output' and replace by 'data' or 'collection'" in lint_ctx.warn_messages
     assert len(lint_ctx.info_messages) == 1
@@ -1546,7 +1549,7 @@ def test_outputs_unknown_tag(lint_ctx):
 
 def test_outputs_unnamed_invalid_name(lint_ctx):
     tool_source = get_xml_tool_source(OUTPUTS_UNNAMED_INVALID_NAME)
-    run_lint_module(lint_ctx, outputs, tool_source)
+    run_lint_module(lint_ctx, output, tool_source)
     assert "3 outputs found." in lint_ctx.info_messages
     assert "Invalid XML: Element 'data': The attribute 'name' is required but missing." in lint_ctx.error_messages
     assert "Invalid XML: Element 'collection': The attribute 'name' is required but missing." in lint_ctx.error_messages
@@ -1562,7 +1565,7 @@ def test_outputs_unnamed_invalid_name(lint_ctx):
 
 def test_outputs_format_input_legacy(lint_ctx):
     tool_source = get_xml_tool_source(OUTPUTS_FORMAT_INPUT_LEGACY)
-    run_lint_module(lint_ctx, outputs, tool_source)
+    run_lint_module(lint_ctx, output, tool_source)
     assert "1 outputs found." in lint_ctx.info_messages
     assert "Using format='input' on data is deprecated. Use the format_source attribute." in lint_ctx.warn_messages
     assert len(lint_ctx.info_messages) == 1
@@ -1573,7 +1576,7 @@ def test_outputs_format_input_legacy(lint_ctx):
 
 def test_outputs_format_input(lint_ctx):
     tool_source = get_xml_tool_source(OUTPUTS_FORMAT_INPUT)
-    run_lint_module(lint_ctx, outputs, tool_source)
+    run_lint_module(lint_ctx, output, tool_source)
     assert "1 outputs found." in lint_ctx.info_messages
     assert "Using format='input' on data is deprecated. Use the format_source attribute." in lint_ctx.error_messages
     assert len(lint_ctx.info_messages) == 1
@@ -1584,7 +1587,7 @@ def test_outputs_format_input(lint_ctx):
 
 def test_outputs_collection_format_source(lint_ctx):
     tool_source = get_xml_tool_source(OUTPUTS_COLLECTION_FORMAT_SOURCE)
-    run_lint_module(lint_ctx, outputs, tool_source)
+    run_lint_module(lint_ctx, output, tool_source)
     assert "Tool data output 'reverse' should use either format_source or format/ext" in lint_ctx.warn_messages
     assert len(lint_ctx.info_messages) == 1
     assert not lint_ctx.valid_messages
@@ -1594,7 +1597,7 @@ def test_outputs_collection_format_source(lint_ctx):
 
 def test_outputs_format_action(lint_ctx):
     tool_source = get_xml_tool_source(OUTPUTS_FORMAT_ACTION)
-    run_lint_module(lint_ctx, outputs, tool_source)
+    run_lint_module(lint_ctx, output, tool_source)
     assert len(lint_ctx.info_messages) == 1
     assert not lint_ctx.valid_messages
     assert not lint_ctx.warn_messages
@@ -1603,7 +1606,7 @@ def test_outputs_format_action(lint_ctx):
 
 def test_outputs_discover_tool_provided_metadata(lint_ctx):
     tool_source = get_xml_tool_source(OUTPUTS_DISCOVER_TOOL_PROVIDED_METADATA)
-    run_lint_module(lint_ctx, outputs, tool_source)
+    run_lint_module(lint_ctx, output, tool_source)
     assert "1 outputs found." in lint_ctx.info_messages
     assert len(lint_ctx.info_messages) == 1
     assert not lint_ctx.valid_messages
@@ -1614,7 +1617,7 @@ def test_outputs_discover_tool_provided_metadata(lint_ctx):
 def test_outputs_duplicated_name_label(lint_ctx):
     """ """
     tool_source = get_xml_tool_source(OUTPUTS_DUPLICATED_NAME_LABEL)
-    run_lint_module(lint_ctx, outputs, tool_source)
+    run_lint_module(lint_ctx, output, tool_source)
     assert "4 outputs found." in lint_ctx.info_messages
     assert len(lint_ctx.info_messages) == 1
     assert not lint_ctx.valid_messages
@@ -2075,6 +2078,30 @@ def test_xml_comments_are_ignored(lint_ctx: LintContext):
         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"]
+
+    tool_source = get_xml_tool_source(CITATIONS_ABSENT)
+    run_lint_module(lint_ctx, citations, tool_source)
+    assert not lint_ctx.warn_messages
+    assert not lint_ctx.info_messages
+    assert not lint_ctx.valid_messages
+    assert not lint_ctx.error_messages
+
+
+def test_skip_by_module(lint_ctx):
+    # add a module name to the skip list -> all linters in this module are skipped
+    lint_ctx.skip_types = ["citations"]
+
+    tool_source = get_xml_tool_source(CITATIONS_ABSENT)
+    run_lint_module(lint_ctx, citations, tool_source)
+    assert not lint_ctx.warn_messages
+    assert not lint_ctx.info_messages
+    assert not lint_ctx.valid_messages
+    assert not lint_ctx.error_messages
+
+
 def test_list_linters():
     linter_names = Linter.list_listers()
     # make sure to add/remove a test for new/removed linters if this number changes
@@ -2095,3 +2122,45 @@ def test_list_linters():
         "XSD",
     ]:
         assert len([x for x in linter_names if x.startswith(prefix)])
+
+
+def test_linter_module_list():
+    linter_modules = submodules.import_submodules(galaxy.tool_util.linters)
+    linter_module_names = [m.__name__.split(".")[-1] for m in linter_modules]
+    linter_module_names = [n for n in linter_module_names if not n.startswith("_")]
+    assert len(linter_module_names) >= 11
+
+    # until 23.2 the linters were implemented as functions lint_xyz contained in a module named xyz
+    # with 24.0 the functions were split in multiple classes (1 per linter message)
+    # in order to keep the skip functionality of lint contexts working (which is used eg in planemo)
+    # with the old names, we now also check for module name if a linter should be skipped
+    # therefore we test here that the module names are not changed
+    # the keys of the following dict represent the linter names before the switch and the values give
+    # the number of linter classes when we switched
+    # so adding new functionality to a linter module is fine. But splitting one or removing functionality
+    # should raise an error here to point to the possible consequences of renaming
+    old_linters = {
+        "citations": 4,
+        "command": 5,
+        "cwl": 9,
+        "general": 17,
+        "help": 6,
+        "inputs": 52,
+        "output": 11,
+        "stdio": 3,
+        "tests": 21,
+        "xml_order": 1,
+    }
+    assert len(set(linter_module_names).intersection(set(old_linters.keys()))) == len(old_linters.keys())
+
+    for module in linter_modules:
+        module_name = module.__name__.split(".")[-1]
+        if module_name not in old_linters:
+            continue
+        linter_cnt = 0
+        for name, value in inspect.getmembers(module):
+            if callable(value) and name.startswith("lint_"):
+                continue
+            elif inspect.isclass(value) and issubclass(value, Linter) and not inspect.isabstract(value):
+                linter_cnt += 1
+        assert linter_cnt >= old_linters[module_name]