Merge pull request #17081 from bernt-matthias/topic/linter-overhaul

Split linters in separate classes

.ci/validate_test_tools.sh

@@ -7,5 +7,7 @@ xsd_path="lib/galaxy/tools/xsd/galaxy.xsd"
 xmllint --noout "$xsd_path"
 
 test_tools_path='test/functional/tools'
-tool_files_list=$(ls "$test_tools_path"/*.xml | grep -v '_conf.xml$')
+# test all test tools except upload.xml which uses a non-standard conditional
+# (without param) which does not survive xsd validation
+tool_files_list=$(ls "$test_tools_path"/*.xml | grep -v '_conf.xml$' | grep -v upload.xml)
 sh scripts/validate_tools.sh $tool_files_list

lib/galaxy/tool_util/lint.py

@@ -45,22 +45,31 @@
 """
 
 import inspect
+from abc import (
+    ABC,
+    abstractmethod,
+)
 from enum import IntEnum
 from typing import (
     Callable,
     List,
     Optional,
     Type,
+    TYPE_CHECKING,
     TypeVar,
     Union,
 )
 
+import galaxy.tool_util.linters
 from galaxy.tool_util.parser import get_tool_source
 from galaxy.util import (
     Element,
     submodules,
 )
 
+if TYPE_CHECKING:
+    from galaxy.tool_util.parser.interface import ToolSource
+
 
 class LintLevel(IntEnum):
     SILENT = 5
@@ -71,14 +80,44 @@ class LintLevel(IntEnum):
     ALL = 0
 
 
+class Linter(ABC):
+    """
+    a linter. needs to define a lint method and the code property.
+    optionally a fix method can be given
+    """
+
+    @classmethod
+    @abstractmethod
+    def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"):
+        """
+        should add at most one message to the lint context
+        """
+        pass
+
+    @classmethod
+    def name(cls) -> str:
+        """
+        get the linter name
+        """
+        return cls.__name__
+
+    @classmethod
+    def list_listers(cls) -> List[str]:
+        """
+        list the names of all linter derived from Linter
+        """
+        return [s.__name__ for s in cls.__subclasses__()]
+
+
 class LintMessage:
     """
     a message from the linter
     """
 
-    def __init__(self, level: str, message: str, **kwargs):
+    def __init__(self, level: str, message: str, linter: Optional[str] = None, **kwargs):
         self.level = level
         self.message = message
+        self.linter = linter
 
     def __eq__(self, other) -> bool:
         """
@@ -95,15 +134,19 @@ def __eq__(self, other) -> bool:
         return False
 
     def __str__(self) -> str:
-        return f".. {self.level.upper()}: {self.message}"
+        if self.linter:
+            linter = f" ({self.linter})"
+        else:
+            linter = ""
+        return f".. {self.level.upper()}{linter}: {self.message}"
 
     def __repr__(self) -> str:
         return f"LintMessage({self.message})"
 
 
 class XMLLintMessageLine(LintMessage):
-    def __init__(self, level: str, message: str, node: Optional[Element] = None):
-        super().__init__(level, message)
+    def __init__(self, level: str, message: str, linter: Optional[str] = None, node: Optional[Element] = None):
+        super().__init__(level, message, linter)
         self.line = None
         if node is not None:
             self.line = node.sourceline
@@ -118,8 +161,8 @@ def __str__(self) -> str:
 
 
 class XMLLintMessageXPath(LintMessage):
-    def __init__(self, level: str, message: str, node: Optional[Element] = None):
-        super().__init__(level, message)
+    def __init__(self, level: str, message: str, linter: Optional[str] = None, node: Optional[Element] = None):
+        super().__init__(level, message, linter)
         self.xpath = None
         if node is not None:
             tool_xml = node.getroottree()
@@ -170,7 +213,8 @@ 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):
-        name = name[len("lint_") :]
+        if name.startswith("lint_"):
+            name = name[len("lint_") :]
         if name in self.skip_types:
             return
 
@@ -187,37 +231,18 @@ def lint(self, name: str, lint_func: Callable[[LintTargetType, "LintContext"], N
         lint_func(lint_target, self)
 
         if self.level < LintLevel.SILENT:
-            # TODO: colorful emoji if in click CLI.
-            if self.error_messages:
-                status = "FAIL"
-            elif self.warn_messages:
-                status = "WARNING"
-            else:
-                status = "CHECK"
-
-            def print_linter_info(printed_linter_info):
-                if printed_linter_info:
-                    return True
-                print(f"Applying linter {name}... {status}")
-                return True
-
-            plf = False
             for message in self.error_messages:
-                plf = print_linter_info(plf)
                 print(f"{message}")
 
             if self.level <= LintLevel.WARN:
                 for message in self.warn_messages:
-                    plf = print_linter_info(plf)
                     print(f"{message}")
 
             if self.level <= LintLevel.INFO:
                 for message in self.info_messages:
-                    plf = print_linter_info(plf)
                     print(f"{message}")
             if self.level <= LintLevel.VALID:
                 for message in self.valid_messages:
-                    plf = print_linter_info(plf)
                     print(f"{message}")
             self.message_list = tmp_message_list + self.message_list
 
@@ -237,22 +262,22 @@ def warn_messages(self) -> List[LintMessage]:
     def error_messages(self) -> List[LintMessage]:
         return [x for x in self.message_list if x.level == "error"]
 
-    def __handle_message(self, level: str, message: str, *args, **kwargs) -> None:
+    def __handle_message(self, level: str, message: str, linter: Optional[str] = None, *args, **kwargs) -> None:
         if args:
             message = message % args
-        self.message_list.append(self.lint_message_class(level=level, message=message, **kwargs))
+        self.message_list.append(self.lint_message_class(level=level, message=message, linter=linter, **kwargs))
 
-    def valid(self, message: str, *args, **kwargs) -> None:
-        self.__handle_message("check", message, *args, **kwargs)
+    def valid(self, message: str, linter: Optional[str] = None, *args, **kwargs) -> None:
+        self.__handle_message("check", message, linter, *args, **kwargs)
 
-    def info(self, message: str, *args, **kwargs) -> None:
-        self.__handle_message("info", message, *args, **kwargs)
+    def info(self, message: str, linter: Optional[str] = None, *args, **kwargs) -> None:
+        self.__handle_message("info", message, linter, *args, **kwargs)
 
-    def error(self, message: str, *args, **kwargs) -> None:
-        self.__handle_message("error", message, *args, **kwargs)
+    def error(self, message: str, linter: Optional[str] = None, *args, **kwargs) -> None:
+        self.__handle_message("error", message, linter, *args, **kwargs)
 
-    def warn(self, message: str, *args, **kwargs) -> None:
-        self.__handle_message("warning", message, *args, **kwargs)
+    def warn(self, message: str, linter: Optional[str] = None, *args, **kwargs) -> None:
+        self.__handle_message("warning", message, linter, *args, **kwargs)
 
     def failed(self, fail_level: Union[LintLevel, str]) -> bool:
         if isinstance(fail_level, str):
@@ -319,12 +344,16 @@ def lint_xml(
 
 def lint_tool_source_with(lint_context, tool_source, extra_modules=None) -> LintContext:
     extra_modules = extra_modules or []
-    import galaxy.tool_util.linters
 
-    tool_xml = getattr(tool_source, "xml_tree", None)
-    tool_type = tool_source.parse_tool_type() or "default"
     linter_modules = submodules.import_submodules(galaxy.tool_util.linters)
     linter_modules.extend(extra_modules)
+    return lint_tool_source_with_modules(lint_context, tool_source, linter_modules)
+
+
+def lint_tool_source_with_modules(lint_context: LintContext, tool_source, linter_modules) -> LintContext:
+    tool_xml = getattr(tool_source, "xml_tree", None)
+    tool_type = tool_source.parse_tool_type() or "default"
+
     for module in linter_modules:
         lint_tool_types = getattr(module, "lint_tool_types", ["default", "manage_data"])
         if not ("*" in lint_tool_types or tool_type in lint_tool_types):
@@ -344,6 +373,8 @@ def lint_tool_source_with(lint_context, tool_source, extra_modules=None) -> Lint
                         lint_context.lint(name, value, tool_xml)
                 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)
     return lint_context
 
 

lib/galaxy/tool_util/linters/citations.py

@@ -1,42 +1,73 @@
-"""This module contains a citation lint function.
+"""This module contains citation linters.
 
 Citations describe references that should be used when consumers
 of the tool publish results.
 """
 
+from typing import TYPE_CHECKING
 
-def lint_citations(tool_xml, lint_ctx):
-    """Ensure tool contains at least one valid citation."""
-    root = tool_xml.find("./citations")
-    if root is None:
-        root = tool_xml.getroot()
-
-    citations = tool_xml.findall("citations")
-    if len(citations) > 1:
-        lint_ctx.error("More than one citation section found, behavior undefined.", node=citations[1])
-        return
-
-    if len(citations) == 0:
-        lint_ctx.warn("No citations found, consider adding citations to your tool.", node=root)
-        return
-
-    valid_citations = 0
-    for citation in citations[0]:
-        if citation.tag != "citation":
-            lint_ctx.warn(
-                f"Unknown tag discovered in citations block [{citation.tag}], will be ignored.", node=citation
-            )
-            continue
-        citation_type = citation.attrib.get("type")
-        if citation_type not in ("bibtex", "doi"):
-            lint_ctx.warn(f"Unknown citation type discovered [{citation_type}], will be ignored.", node=citation)
-            continue
-        if citation.text is None or not citation.text.strip():
-            lint_ctx.error(f"Empty {citation_type} citation.", node=citation)
-            continue
-        valid_citations += 1
-
-    if valid_citations > 0:
-        lint_ctx.valid(f"Found {valid_citations} likely valid citations.", node=root)
-    else:
-        lint_ctx.warn("Found no valid citations.", node=root)
+from galaxy.tool_util.lint import Linter
+
+if TYPE_CHECKING:
+    from galaxy.tool_util.lint import LintContext
+    from galaxy.tool_util.parser.interface import ToolSource
+
+
+class CitationsMissing(Linter):
+    @classmethod
+    def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"):
+        tool_xml = getattr(tool_source, "xml_tree", None)
+        if not tool_xml:
+            return
+        root = tool_xml.find("./citations")
+        if root is None:
+            root = tool_xml.getroot()
+        citations = tool_xml.findall("citations")
+        if len(citations) == 0:
+            lint_ctx.warn("No citations found, consider adding citations to your tool.", linter=cls.name(), node=root)
+
+
+class CitationsNoText(Linter):
+    @classmethod
+    def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"):
+        tool_xml = getattr(tool_source, "xml_tree", None)
+        if not tool_xml:
+            return
+        citations = tool_xml.find("citations")
+        if citations is None:
+            return
+        for citation in citations:
+            citation_type = citation.attrib.get("type")
+            if citation_type in ["doi", "bibtex"] and (citation.text is None or not citation.text.strip()):
+                lint_ctx.error(f"Empty {citation_type} citation.", linter=cls.name(), node=citation)
+
+
+class CitationsFound(Linter):
+    @classmethod
+    def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"):
+        tool_xml = getattr(tool_source, "xml_tree", None)
+        if not tool_xml:
+            return
+        root = tool_xml.find("./citations")
+        if root is None:
+            root = tool_xml.getroot()
+        citations = tool_xml.find("citations")
+
+        if citations is not None and len(citations) > 0:
+            lint_ctx.valid(f"Found {len(citations)} citations.", linter=cls.name(), node=root)
+
+
+class CitationsNoValid(Linter):
+    @classmethod
+    def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"):
+        tool_xml = getattr(tool_source, "xml_tree", None)
+        if not tool_xml:
+            return
+        root = tool_xml.find("./citations")
+        if root is None:
+            root = tool_xml.getroot()
+        citations = tool_xml.findall("citations")
+        if len(citations) != 1:
+            return
+        if len(citations[0]) == 0:
+            lint_ctx.warn("Found no valid citations.", linter=cls.name(), node=root)