From 8eb0ef0f8ea79224bd76bb396af925cc15c3747c Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Fri, 24 Nov 2023 16:23:50 +0100 Subject: [PATCH 01/32] split stdio linters in separate classes mainly serves as a proof of concept - removes the attribute check for regex and exit_code tags and subtag check for stdio since those are covered already by the xsd linter --- lib/galaxy/tool_util/lint.py | 43 ++++++++- lib/galaxy/tool_util/linters/stdio.py | 114 ++++++++++++++--------- test/unit/tool_util/test_tool_linters.py | 15 ++- 3 files changed, 118 insertions(+), 54 deletions(-) diff --git a/lib/galaxy/tool_util/lint.py b/lib/galaxy/tool_util/lint.py index 44ab28b9790e..d4e89a7c0143 100644 --- a/lib/galaxy/tool_util/lint.py +++ b/lib/galaxy/tool_util/lint.py @@ -45,12 +45,17 @@ """ import inspect +from abc import ( + ABC, + abstractmethod, +) from enum import IntEnum from typing import ( Callable, List, Optional, Type, + TYPE_CHECKING, TypeVar, Union, ) @@ -61,6 +66,9 @@ submodules, ) +if TYPE_CHECKING: + from galaxy.tool_util.parser.interface import ToolSource + class LintLevel(IntEnum): SILENT = 5 @@ -71,6 +79,27 @@ 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 + """ + + @property + @abstractmethod + def code(self) -> str: + pass + + @classmethod + @abstractmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + pass + + @classmethod # noqa: B027 + def fix(cls): # noqa: B027 + pass + + class LintMessage: """ a message from the linter @@ -170,7 +199,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 @@ -321,10 +351,15 @@ def lint_tool_source_with(lint_context, tool_source, extra_modules=None) -> Lint 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 +379,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): + lint_context.lint(name, value.lint, tool_source) return lint_context diff --git a/lib/galaxy/tool_util/linters/stdio.py b/lib/galaxy/tool_util/linters/stdio.py index 981882bbde39..b0e330af3bc1 100644 --- a/lib/galaxy/tool_util/linters/stdio.py +++ b/lib/galaxy/tool_util/linters/stdio.py @@ -1,68 +1,90 @@ """This module contains a linting functions for tool error detection.""" import re +from typing import TYPE_CHECKING -from galaxy.util import etree -from .command import get_command +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 -def lint_stdio(tool_source, lint_ctx): - tool_xml = getattr(tool_source, "xml_tree", None) - if not tool_xml: - # Can only lint XML tools at this point. - # Should probably use tool_source.parse_stdio() to abstract away XML details - return - stdios = tool_xml.findall("./stdio") if tool_xml else [] - # determine node to report for general problems with stdio - tool_node = tool_xml.find("./stdio") - if tool_node is None: - tool_node = tool_xml.getroot() - if not stdios: - command = get_command(tool_xml) if tool_xml else None +class StdIOAbsenceLegacy(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + # Can only lint XML tools at this point. + # Should probably use tool_source.parse_stdio() to abstract away XML details + return + stdios = tool_xml.findall("./stdio") if tool_xml else [] + if stdios: + continue + tool_node = tool_xml.getroot() + command = tool_xml.find("./command") if command is None or not command.get("detect_errors"): if tool_source.parse_profile() <= "16.01": lint_ctx.info( "No stdio definition found, tool indicates error conditions with output written to stderr.", node=tool_node, ) - else: + + +class StdIOAbsence(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + # Can only lint XML tools at this point. + # Should probably use tool_source.parse_stdio() to abstract away XML details + return + stdios = tool_xml.findall("./stdio") if tool_xml else [] + if stdios: + continue + tool_node = tool_xml.getroot() + command = tool_xml.find("./command") + if command is None or not command.get("detect_errors"): + if tool_source.parse_profile() > 16.01: lint_ctx.info( "No stdio definition found, tool indicates error conditions with non-zero exit codes.", node=tool_node, ) - return - if len(stdios) > 1: - lint_ctx.error("More than one stdio tag found, behavior undefined.", node=stdios[1]) - return - stdio = stdios[0] - for child in list(stdio): - if child.tag is etree.Comment: - continue - if child.tag == "regex": - _lint_regex(tool_xml, child, lint_ctx) - elif child.tag == "exit_code": - _lint_exit_code(tool_xml, child, lint_ctx) - else: - message = "Unknown stdio child tag discovered [%s]. " - message += "Valid options are exit_code and regex." - lint_ctx.warn(message % child.tag, node=child) +class StdIOMultiple(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + # Can only lint XML tools at this point. + # Should probably use tool_source.parse_stdio() to abstract away XML details + return + stdios = tool_xml.findall("./stdio") if tool_xml else [] + + if len(stdios) > 1: + lint_ctx.error("More than one stdio tag found, behavior undefined.", node=stdios[1]) + return -def _lint_exit_code(tool_xml, child, lint_ctx): - for key in child.attrib.keys(): - if key not in ["description", "level", "range"]: - lint_ctx.warn(f"Unknown attribute [{key}] encountered on exit_code tag.", node=child) +class StdIORegex(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + # Can only lint XML tools at this point. + # Should probably use tool_source.parse_stdio() to abstract away XML details + return + stdios = tool_xml.findall("./stdio") if tool_xml else [] + if len(stdios) != 1: + return -def _lint_regex(tool_xml, child, lint_ctx): - for key in child.attrib.keys(): - if key not in ["description", "level", "match", "source"]: - lint_ctx.warn(f"Unknown attribute [{key}] encountered on regex tag.", node=child) - match = child.attrib.get("match") - if match: - try: - re.compile(match) - except Exception as e: - lint_ctx.error(f"Match '{match}' is no valid regular expression: {str(e)}", node=child) + stdio = stdios[0] + for child in list(stdio): + if child.tag == "regex": + match = child.attrib.get("match") + if match: + try: + re.compile(match) + except Exception as e: + lint_ctx.error(f"Match '{match}' is no valid regular expression: {str(e)}", node=child) diff --git a/test/unit/tool_util/test_tool_linters.py b/test/unit/tool_util/test_tool_linters.py index 7b2f134192a9..8825606fa27c 100644 --- a/test/unit/tool_util/test_tool_linters.py +++ b/test/unit/tool_util/test_tool_linters.py @@ -5,6 +5,7 @@ from galaxy.tool_util.lint import ( lint_tool_source_with, + lint_tool_source_with_modules, lint_xml_with, LintContext, XMLLintMessageLine, @@ -920,6 +921,10 @@ def get_tool_xml_exact(xml_string: str): 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, [lint_module]) + + def run_lint(lint_ctx, lint_func, lint_target): lint_ctx.lint(name="test_lint", lint_func=lint_func, lint_target=lint_target) # check if the lint messages have the line @@ -1585,7 +1590,7 @@ def test_outputs_duplicated_name_label(lint_ctx): def test_stdio_default_for_default_profile(lint_ctx): tool_source = get_xml_tool_source(STDIO_DEFAULT_FOR_DEFAULT_PROFILE) - run_lint(lint_ctx, stdio.lint_stdio, tool_source) + run_lint_module(lint_ctx, stdio, tool_source) assert ( "No stdio definition found, tool indicates error conditions with output written to stderr." in lint_ctx.info_messages @@ -1598,7 +1603,7 @@ def test_stdio_default_for_default_profile(lint_ctx): def test_stdio_default_for_nonlegacy_profile(lint_ctx): tool_source = get_xml_tool_source(STDIO_DEFAULT_FOR_NONLEGACY_PROFILE) - run_lint(lint_ctx, stdio.lint_stdio, tool_source) + run_lint_module(lint_ctx, stdio, tool_source) assert ( "No stdio definition found, tool indicates error conditions with non-zero exit codes." in lint_ctx.info_messages ) @@ -1610,7 +1615,7 @@ def test_stdio_default_for_nonlegacy_profile(lint_ctx): def test_stdio_multiple_stdio(lint_ctx): tool_source = get_xml_tool_source(STDIO_MULTIPLE_STDIO) - run_lint(lint_ctx, stdio.lint_stdio, tool_source) + run_lint_module(lint_ctx, stdio, tool_source) assert "More than one stdio tag found, behavior undefined." in lint_ctx.error_messages assert not lint_ctx.info_messages assert not lint_ctx.valid_messages @@ -1620,7 +1625,7 @@ def test_stdio_multiple_stdio(lint_ctx): def test_stdio_invalid_child_or_attrib(lint_ctx): tool_source = get_xml_tool_source(STDIO_INVALID_CHILD_OR_ATTRIB) - run_lint(lint_ctx, stdio.lint_stdio, tool_source) + run_lint_module(lint_ctx, stdio, tool_source) assert ( "Unknown stdio child tag discovered [reqex]. Valid options are exit_code and regex." in lint_ctx.warn_messages ) @@ -1634,7 +1639,7 @@ def test_stdio_invalid_child_or_attrib(lint_ctx): def test_stdio_invalid_match(lint_ctx): tool_source = get_xml_tool_source(STDIO_INVALID_MATCH) - run_lint(lint_ctx, stdio.lint_stdio, tool_source) + run_lint_module(lint_ctx, stdio, tool_source) assert ( "Match '[' is no valid regular expression: unterminated character set at position 0" in lint_ctx.error_messages ) From 0fbe61766c312cf1f9fafc27ed9ad5342f942832 Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Sat, 25 Nov 2023 17:23:06 +0100 Subject: [PATCH 02/32] remove linter code and fixes --- lib/galaxy/tool_util/lint.py | 7 +------ lib/galaxy/tool_util/linters/stdio.py | 10 ++++++---- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/lib/galaxy/tool_util/lint.py b/lib/galaxy/tool_util/lint.py index d4e89a7c0143..f5cf0988c3e4 100644 --- a/lib/galaxy/tool_util/lint.py +++ b/lib/galaxy/tool_util/lint.py @@ -85,11 +85,6 @@ class Linter(ABC): optionally a fix method can be given """ - @property - @abstractmethod - def code(self) -> str: - pass - @classmethod @abstractmethod def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): @@ -105,7 +100,7 @@ class LintMessage: a message from the linter """ - def __init__(self, level: str, message: str, **kwargs): + def __init__(self, level: str, message: str): self.level = level self.message = message diff --git a/lib/galaxy/tool_util/linters/stdio.py b/lib/galaxy/tool_util/linters/stdio.py index b0e330af3bc1..3e1bf619b279 100644 --- a/lib/galaxy/tool_util/linters/stdio.py +++ b/lib/galaxy/tool_util/linters/stdio.py @@ -2,6 +2,8 @@ import re from typing import TYPE_CHECKING +from packaging.version import Version + from galaxy.tool_util.lint import Linter if TYPE_CHECKING: @@ -19,11 +21,11 @@ def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): return stdios = tool_xml.findall("./stdio") if tool_xml else [] if stdios: - continue + return tool_node = tool_xml.getroot() command = tool_xml.find("./command") if command is None or not command.get("detect_errors"): - if tool_source.parse_profile() <= "16.01": + if Version(tool_source.parse_profile()) <= Version("16.01"): lint_ctx.info( "No stdio definition found, tool indicates error conditions with output written to stderr.", node=tool_node, @@ -40,11 +42,11 @@ def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): return stdios = tool_xml.findall("./stdio") if tool_xml else [] if stdios: - continue + return tool_node = tool_xml.getroot() command = tool_xml.find("./command") if command is None or not command.get("detect_errors"): - if tool_source.parse_profile() > 16.01: + if Version(tool_source.parse_profile()) > Version("16.01"): lint_ctx.info( "No stdio definition found, tool indicates error conditions with non-zero exit codes.", node=tool_node, From 8d4394711f939f3a4500e8169b824e6778b17c4b Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Sat, 25 Nov 2023 20:48:08 +0100 Subject: [PATCH 03/32] split general and citations linter - removing those messages from citations that are covered by xsd checks --- lib/galaxy/tool_util/linters/citations.py | 113 +++++++---- lib/galaxy/tool_util/linters/general.py | 237 ++++++++++++++++------ test/unit/tool_util/test_tool_linters.py | 87 ++++---- 3 files changed, 294 insertions(+), 143 deletions(-) diff --git a/lib/galaxy/tool_util/linters/citations.py b/lib/galaxy/tool_util/linters/citations.py index 562691c15213..e799e7773528 100644 --- a/lib/galaxy/tool_util/linters/citations.py +++ b/lib/galaxy/tool_util/linters/citations.py @@ -1,42 +1,83 @@ -"""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 +from galaxy.tool_util.lint import Linter -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) +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.", node=root) + + +class CitationsMultiple(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.findall("citations") + if len(citations) > 1: + lint_ctx.error("More than one citation section found, behavior undefined.", node=citations[1]) + + +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.", 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.", 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.", node=root) diff --git a/lib/galaxy/tool_util/linters/general.py b/lib/galaxy/tool_util/linters/general.py index ee2df7f004d1..c3f1fb0a88e0 100644 --- a/lib/galaxy/tool_util/linters/general.py +++ b/lib/galaxy/tool_util/linters/general.py @@ -1,87 +1,204 @@ """This module contains linting functions for general aspects of the tool.""" import re +from typing import ( + Tuple, + TYPE_CHECKING, +) +from galaxy.tool_util.lint import Linter from galaxy.tool_util.version import ( LegacyVersion, parse_version, ) -ERROR_VERSION_MSG = "Tool version is missing or empty." -WARN_VERSION_MSG = "Tool version [%s] is not compliant with PEP 440." -VALID_VERSION_MSG = "Tool defines a version [%s]." - -ERROR_NAME_MSG = "Tool name is missing or empty." -VALID_NAME_MSG = "Tool defines a name [%s]." - -ERROR_ID_MSG = "Tool does not define an id attribute." -VALID_ID_MSG = "Tool defines an id [%s]." +if TYPE_CHECKING: + from galaxy.tool_util.lint import LintContext + from galaxy.tool_util.parser.interface import ToolSource + from galaxy.util.etree import ( + Element, + ElementTree, + ) PROFILE_PATTERN = re.compile(r"^[12]\d\.\d{1,2}$") -PROFILE_INFO_DEFAULT_MSG = "Tool targets 16.01 Galaxy profile." -PROFILE_INFO_SPECIFIED_MSG = "Tool specifies profile version [%s]." -PROFILE_INVALID_MSG = "Tool specifies an invalid profile version [%s]." -WARN_WHITESPACE_MSG = "%s contains whitespace, this may cause errors: [%s]." -WARN_WHITESPACE_PRESUFFIX = "%s is pre/suffixed by whitespace, this may cause errors: [%s]." -WARN_ID_WHITESPACE_MSG = "Tool ID contains whitespace - this is discouraged: [%s]." lint_tool_types = ["*"] -def lint_general(tool_source, lint_ctx): - """Check tool version, name, and id.""" - # determine line to report for general problems with outputs +def _tool_xml_and_root(tool_source: "ToolSource") -> Tuple["ElementTree", "Element"]: tool_xml = getattr(tool_source, "xml_tree", None) if tool_xml: tool_node = tool_xml.getroot() else: tool_node = None - version = tool_source.parse_version() or "" - parsed_version = parse_version(version) - if not version: - lint_ctx.error(ERROR_VERSION_MSG, node=tool_node) - elif isinstance(parsed_version, LegacyVersion): - lint_ctx.warn(WARN_VERSION_MSG % version, node=tool_node) - elif version != version.strip(): - lint_ctx.warn(WARN_WHITESPACE_PRESUFFIX % ("Tool version", version), node=tool_node) - else: - lint_ctx.valid(VALID_VERSION_MSG % version, node=tool_node) + return tool_xml, tool_node - name = tool_source.parse_name() - if not name: - lint_ctx.error(ERROR_NAME_MSG, node=tool_node) - elif name != name.strip(): - lint_ctx.warn(WARN_WHITESPACE_PRESUFFIX % ("Tool name", name), node=tool_node) - else: - lint_ctx.valid(VALID_NAME_MSG % name, node=tool_node) - tool_id = tool_source.parse_id() - if not tool_id: - lint_ctx.error(ERROR_ID_MSG, node=tool_node) - elif re.search(r"\s", tool_id): - lint_ctx.warn(WARN_ID_WHITESPACE_MSG % tool_id, node=tool_node) - else: - lint_ctx.valid(VALID_ID_MSG % tool_id, node=tool_node) - - profile = tool_source.parse_profile() - profile_valid = PROFILE_PATTERN.match(profile) is not None - if not profile_valid: - lint_ctx.error(PROFILE_INVALID_MSG % profile, node=tool_node) - elif profile == "16.01": - lint_ctx.valid(PROFILE_INFO_DEFAULT_MSG, node=tool_node) - else: - lint_ctx.valid(PROFILE_INFO_SPECIFIED_MSG % profile, node=tool_node) +class ToolVersionMissing(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml, tool_node = _tool_xml_and_root(tool_source) + version = tool_source.parse_version() or "" + if not version: + lint_ctx.error("Tool version is missing or empty.", node=tool_node) + + +class ToolVersionPEP404(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml, tool_node = _tool_xml_and_root(tool_source) + version = tool_source.parse_version() or "" + parsed_version = parse_version(version) + if version and isinstance(parsed_version, LegacyVersion): + lint_ctx.warn(f"Tool version [{version}] is not compliant with PEP 440.", node=tool_node) + + +class ToolVersionWhitespace(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml, tool_node = _tool_xml_and_root(tool_source) + version = tool_source.parse_version() or "" + if version != version.strip(): + lint_ctx.warn( + f"Tool version is pre/suffixed by whitespace, this may cause errors: [{version}].", node=tool_node + ) + + +class ToolVersionValid(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml, tool_node = _tool_xml_and_root(tool_source) + version = tool_source.parse_version() or "" + parsed_version = parse_version(version) + if version and not isinstance(parsed_version, LegacyVersion) and version == version.strip(): + lint_ctx.valid(f"Tool defines a version [{version}].", node=tool_node) + + +class ToolNameMissing(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + _, tool_node = _tool_xml_and_root(tool_source) + name = tool_source.parse_name() + if not name: + lint_ctx.error("Tool name is missing or empty.", node=tool_node) + + +class ToolNameWhitespace(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + _, tool_node = _tool_xml_and_root(tool_source) + name = tool_source.parse_name() + if name and name != name.strip(): + lint_ctx.warn(f"Tool name is pre/suffixed by whitespace, this may cause errors: [{name}].", node=tool_node) + + +class ToolNameValid(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + _, tool_node = _tool_xml_and_root(tool_source) + name = tool_source.parse_name() + if name and name == name.strip(): + lint_ctx.valid(f"Tool defines a name [{name}].", node=tool_node) - requirements, containers, resource_requirements = tool_source.parse_requirements_and_containers() - for r in requirements: - if r.type == "package": + +class ToolIDMissing(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + _, tool_node = _tool_xml_and_root(tool_source) + tool_id = tool_source.parse_id() + if not tool_id: + lint_ctx.error("Tool does not define an id attribute.", node=tool_node) + + +class ToolIDWhitespace(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + _, tool_node = _tool_xml_and_root(tool_source) + tool_id = tool_source.parse_id() + if tool_id and re.search(r"\s", tool_id): + lint_ctx.warn(f"Tool ID contains whitespace - this is discouraged: [{tool_id}].", node=tool_node) + + +class ToolIDValid(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + _, tool_node = _tool_xml_and_root(tool_source) + tool_id = tool_source.parse_id() + if tool_id and not re.search(r"\s", tool_id): + lint_ctx.valid(f"Tool defines an id [{tool_id}].", node=tool_node) + + +class ToolProfileInvalid(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + _, tool_node = _tool_xml_and_root(tool_source) + profile = tool_source.parse_profile() + profile_valid = PROFILE_PATTERN.match(profile) is not None + if not profile_valid: + lint_ctx.error(f"Tool specifies an invalid profile version [{profile}].", node=tool_node) + + +class ToolProfileLegacy(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + _, tool_node = _tool_xml_and_root(tool_source) + profile = tool_source.parse_profile() + profile_valid = PROFILE_PATTERN.match(profile) is not None + if profile_valid and profile == "16.01": + lint_ctx.valid("Tool targets 16.01 Galaxy profile.", node=tool_node) + + +class ToolProfileValid(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + _, tool_node = _tool_xml_and_root(tool_source) + profile = tool_source.parse_profile() + profile_valid = PROFILE_PATTERN.match(profile) is not None + if profile_valid and profile != "16.01": + lint_ctx.valid(f"Tool specifies profile version [{profile}].", node=tool_node) + + +class RequirementNameMissing(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + _, tool_node = _tool_xml_and_root(tool_source) + requirements, containers, resource_requirements = tool_source.parse_requirements_and_containers() + for r in requirements: + if r.type != "package": + continue if not r.name: lint_ctx.error("Requirement without name found") + + +class RequirementVersionMissing(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + _, tool_node = _tool_xml_and_root(tool_source) + requirements, containers, resource_requirements = tool_source.parse_requirements_and_containers() + for r in requirements: + if r.type != "package": + continue if not r.version: lint_ctx.warn(f"Requirement {r.name} defines no version") - # Warn requirement attributes with leading/trailing whitespace: - elif r.version != r.version.strip(): - lint_ctx.warn(WARN_WHITESPACE_MSG % ("Requirement version", r.version)) - for rr in resource_requirements: - if rr.runtime_required: - lint_ctx.warn("Expressions in resource requirement not supported yet") + + +class RequirementVersionWhitespace(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + _, tool_node = _tool_xml_and_root(tool_source) + requirements, containers, resource_requirements = tool_source.parse_requirements_and_containers() + for r in requirements: + if r.type != "package": + continue + if r.version and r.version != r.version.strip(): + lint_ctx.warn(f"Requirement version contains whitespace, this may cause errors: [{r.version}].") + + +class RessourceRequirementExpression(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + _, tool_node = _tool_xml_and_root(tool_source) + requirements, containers, resource_requirements = tool_source.parse_requirements_and_containers() + for rr in resource_requirements: + if rr.runtime_required: + lint_ctx.warn("Expressions in resource requirement not supported yet") diff --git a/test/unit/tool_util/test_tool_linters.py b/test/unit/tool_util/test_tool_linters.py index 8825606fa27c..a9be82463c26 100644 --- a/test/unit/tool_util/test_tool_linters.py +++ b/test/unit/tool_util/test_tool_linters.py @@ -46,8 +46,6 @@ CITATIONS_ERRORS = """ - - @@ -929,23 +927,21 @@ def run_lint(lint_ctx, lint_func, lint_target): lint_ctx.lint(name="test_lint", lint_func=lint_func, lint_target=lint_target) # check if the lint messages have the line for message in lint_ctx.message_list: - if lint_func != general.lint_general: - assert message.line is not None, f"No context found for message: {message.message}" + assert message.line is not None, f"No context found for message: {message.message}" def test_citations_multiple(lint_ctx): - tool_xml_tree = get_xml_tree(CITATIONS_MULTIPLE) - run_lint(lint_ctx, citations.lint_citations, tool_xml_tree) - assert "More than one citation section found, behavior undefined." in lint_ctx.error_messages + tool_source = get_xml_tool_source(CITATIONS_MULTIPLE) + run_lint_module(lint_ctx, citations, tool_source) + assert lint_ctx.error_messages == ["More than one citation section found, behavior undefined."] assert not lint_ctx.info_messages assert not lint_ctx.valid_messages assert not lint_ctx.warn_messages - assert len(lint_ctx.error_messages) == 1 def test_citations_absent(lint_ctx): - tool_xml_tree = get_xml_tree(CITATIONS_ABSENT) - run_lint(lint_ctx, citations.lint_citations, tool_xml_tree) + tool_source = get_xml_tool_source(CITATIONS_ABSENT) + run_lint_module(lint_ctx, citations, tool_source) assert lint_ctx.warn_messages == ["No citations found, consider adding citations to your tool."] assert not lint_ctx.info_messages assert not lint_ctx.valid_messages @@ -953,21 +949,18 @@ def test_citations_absent(lint_ctx): def test_citations_errors(lint_ctx): - tool_xml_tree = get_xml_tree(CITATIONS_ERRORS) - run_lint(lint_ctx, citations.lint_citations, tool_xml_tree) - assert "Unknown tag discovered in citations block [nonsense], will be ignored." in lint_ctx.warn_messages - assert "Unknown citation type discovered [hoerensagen], will be ignored." in lint_ctx.warn_messages - assert "Empty doi citation." in lint_ctx.error_messages - assert "Found no valid citations." in lint_ctx.warn_messages - assert len(lint_ctx.warn_messages) == 3 + tool_source = get_xml_tool_source(CITATIONS_ERRORS) + run_lint_module(lint_ctx, citations, tool_source) + assert lint_ctx.error_messages == ["Empty doi citation."] + assert not lint_ctx.warn_messages assert not lint_ctx.info_messages - assert not lint_ctx.valid_messages + assert len(lint_ctx.valid_messages) == 1 def test_citations_valid(lint_ctx): - tool_xml_tree = get_xml_tree(CITATIONS_VALID) - run_lint(lint_ctx, citations.lint_citations, tool_xml_tree) - assert "Found 1 likely valid citations." in lint_ctx.valid_messages + tool_source = get_xml_tool_source(CITATIONS_VALID) + run_lint_module(lint_ctx, citations, tool_source) + assert "Found 1 citations." in lint_ctx.valid_messages assert len(lint_ctx.valid_messages) == 1 assert not lint_ctx.info_messages assert not lint_ctx.error_messages @@ -975,7 +968,7 @@ def test_citations_valid(lint_ctx): def test_command_multiple(lint_ctx): tool_xml_tree = get_xml_tree(COMMAND_MULTIPLE) - run_lint(lint_ctx, command.lint_command, tool_xml_tree) + run_lint_module(lint_ctx, command, tool_xml_tree) assert "More than one command tag found, behavior undefined." in lint_ctx.error_messages assert len(lint_ctx.error_messages) == 1 assert not lint_ctx.info_messages @@ -985,20 +978,20 @@ def test_command_multiple(lint_ctx): def test_command_missing(lint_ctx): tool_xml_tree = get_xml_tree(COMMAND_MISSING) - run_lint(lint_ctx, command.lint_command, tool_xml_tree) + run_lint_module(lint_ctx, command, tool_xml_tree) assert "No command tag found, must specify a command template to execute." in lint_ctx.error_messages def test_command_todo(lint_ctx): tool_xml_tree = get_xml_tree(COMMAND_TODO) - run_lint(lint_ctx, command.lint_command, tool_xml_tree) + run_lint_module(lint_ctx, command, tool_xml_tree) assert "Tool contains a command." in lint_ctx.info_messages assert "Command template contains TODO text." in lint_ctx.warn_messages def test_command_detect_errors_interpreter(lint_ctx): tool_xml_tree = get_xml_tree(COMMAND_DETECT_ERRORS_INTERPRETER) - run_lint(lint_ctx, command.lint_command, tool_xml_tree) + run_lint_module(lint_ctx, command, tool_xml_tree) assert "Command uses deprecated 'interpreter' attribute." in lint_ctx.warn_messages assert "Tool contains a command with interpreter of type [python]." in lint_ctx.info_messages assert "Unknown detect_errors attribute [nonsense]" in lint_ctx.warn_messages @@ -1007,7 +1000,7 @@ def test_command_detect_errors_interpreter(lint_ctx): def test_general_missing_tool_id_name_version(lint_ctx): tool_source = get_xml_tool_source(GENERAL_MISSING_TOOL_ID_NAME_VERSION) - run_lint(lint_ctx, general.lint_general, tool_source) + run_lint_module(lint_ctx, general, tool_source) assert "Tool version is missing or empty." in lint_ctx.error_messages assert "Tool name is missing or empty." in lint_ctx.error_messages assert "Tool does not define an id attribute." in lint_ctx.error_messages @@ -1016,7 +1009,7 @@ def test_general_missing_tool_id_name_version(lint_ctx): def test_general_whitespace_in_versions_and_names(lint_ctx): tool_source = get_xml_tool_source(GENERAL_WHITESPACE_IN_VERSIONS_AND_NAMES) - run_lint(lint_ctx, general.lint_general, tool_source) + run_lint_module(lint_ctx, general, tool_source) assert "Tool version is pre/suffixed by whitespace, this may cause errors: [ 1.0.1 ]." in lint_ctx.warn_messages assert "Tool name is pre/suffixed by whitespace, this may cause errors: [ BWA Mapper ]." in lint_ctx.warn_messages assert "Requirement version contains whitespace, this may cause errors: [ 1.2.5 ]." in lint_ctx.warn_messages @@ -1026,7 +1019,7 @@ def test_general_whitespace_in_versions_and_names(lint_ctx): def test_general_requirement_without_version(lint_ctx): tool_source = get_xml_tool_source(GENERAL_REQUIREMENT_WO_VERSION) - run_lint(lint_ctx, general.lint_general, tool_source) + run_lint_module(lint_ctx, general, tool_source) assert "Tool version [1.0.1blah] is not compliant with PEP 440." in lint_ctx.warn_messages assert "Requirement bwa defines no version" in lint_ctx.warn_messages assert "Requirement without name found" in lint_ctx.error_messages @@ -1041,7 +1034,7 @@ def test_general_requirement_without_version(lint_ctx): def test_general_valid(lint_ctx): tool_source = get_xml_tool_source(GENERAL_VALID) - run_lint(lint_ctx, general.lint_general, tool_source) + run_lint_module(lint_ctx, general, tool_source) assert "Tool defines a version [1.0+galaxy1]." in lint_ctx.valid_messages assert "Tool specifies profile version [21.09]." in lint_ctx.valid_messages assert "Tool defines an id [valid_id]." in lint_ctx.valid_messages @@ -1054,7 +1047,7 @@ def test_general_valid(lint_ctx): def test_general_valid_new_profile_fmt(lint_ctx): tool_source = get_xml_tool_source(GENERAL_VALID_NEW_PROFILE_FMT) - run_lint(lint_ctx, general.lint_general, tool_source) + run_lint_module(lint_ctx, general, tool_source) assert "Tool defines a version [1.0+galaxy1]." in lint_ctx.valid_messages assert "Tool specifies profile version [23.0]." in lint_ctx.valid_messages assert "Tool defines an id [valid_id]." in lint_ctx.valid_messages @@ -1067,7 +1060,7 @@ def test_general_valid_new_profile_fmt(lint_ctx): def test_help_multiple(lint_ctx): tool_xml_tree = get_xml_tree(HELP_MULTIPLE) - run_lint(lint_ctx, help.lint_help, tool_xml_tree) + run_lint_module(lint_ctx, help, tool_xml_tree) assert "More than one help section found, behavior undefined." in lint_ctx.error_messages assert not lint_ctx.info_messages assert not lint_ctx.valid_messages @@ -1077,7 +1070,7 @@ def test_help_multiple(lint_ctx): def test_help_absent(lint_ctx): tool_xml_tree = get_xml_tree(HELP_ABSENT) - run_lint(lint_ctx, help.lint_help, tool_xml_tree) + run_lint_module(lint_ctx, help, tool_xml_tree) assert "No help section found, consider adding a help section to your tool." in lint_ctx.warn_messages assert not lint_ctx.info_messages assert not lint_ctx.valid_messages @@ -1087,7 +1080,7 @@ def test_help_absent(lint_ctx): def test_help_empty(lint_ctx): tool_xml_tree = get_xml_tree(HELP_EMPTY) - run_lint(lint_ctx, help.lint_help, tool_xml_tree) + run_lint_module(lint_ctx, help, tool_xml_tree) assert "Help section appears to be empty." in lint_ctx.warn_messages assert not lint_ctx.info_messages assert not lint_ctx.valid_messages @@ -1097,7 +1090,7 @@ def test_help_empty(lint_ctx): def test_help_todo(lint_ctx): tool_xml_tree = get_xml_tree(HELP_TODO) - run_lint(lint_ctx, help.lint_help, tool_xml_tree) + run_lint_module(lint_ctx, help, tool_xml_tree) assert "Tool contains help section." in lint_ctx.valid_messages assert "Help contains valid reStructuredText." in lint_ctx.valid_messages assert "Help contains TODO text." in lint_ctx.warn_messages @@ -1109,7 +1102,7 @@ def test_help_todo(lint_ctx): def test_help_invalid_rst(lint_ctx): tool_xml_tree = get_xml_tree(HELP_INVALID_RST) - run_lint(lint_ctx, help.lint_help, tool_xml_tree) + run_lint_module(lint_ctx, help, tool_xml_tree) assert "Tool contains help section." in lint_ctx.valid_messages assert ( "Invalid reStructuredText found in help - [:2: (WARNING/2) Inline strong start-string without end-string.\n]." @@ -1651,7 +1644,7 @@ def test_stdio_invalid_match(lint_ctx): def test_tests_absent(lint_ctx): tool_xml_tree = get_xml_tree(TESTS_ABSENT) - run_lint(lint_ctx, tests.lint_tests, tool_xml_tree) + run_lint_module(lint_ctx, tests, tool_xml_tree) assert "No tests found, most tools should define test cases." in lint_ctx.warn_messages assert not lint_ctx.info_messages assert not lint_ctx.valid_messages @@ -1661,7 +1654,7 @@ def test_tests_absent(lint_ctx): def test_tests_data_source(lint_ctx): tool_xml_tree = get_xml_tree(TESTS_ABSENT_DATA_SOURCE) - run_lint(lint_ctx, tests.lint_tests, tool_xml_tree) + run_lint_module(lint_ctx, tests, tool_xml_tree) assert "No tests found, that should be OK for data_sources." in lint_ctx.info_messages assert len(lint_ctx.info_messages) == 1 assert not lint_ctx.valid_messages @@ -1671,7 +1664,7 @@ def test_tests_data_source(lint_ctx): def test_tests_param_output_names(lint_ctx): tool_xml_tree = get_xml_tree(TESTS_PARAM_OUTPUT_NAMES) - run_lint(lint_ctx, tests.lint_tests, tool_xml_tree) + run_lint_module(lint_ctx, tests, tool_xml_tree) assert "1 test(s) found." in lint_ctx.valid_messages assert "Test 1: Found test param tag without a name defined." in lint_ctx.error_messages assert "Test 1: Test param non_existent_test_name not found in the inputs" in lint_ctx.error_messages @@ -1693,7 +1686,7 @@ def test_tests_param_output_names(lint_ctx): def test_tests_expect_failure_output(lint_ctx): tool_xml_tree = get_xml_tree(TESTS_EXPECT_FAILURE_OUTPUT) - run_lint(lint_ctx, tests.lint_tests, tool_xml_tree) + run_lint_module(lint_ctx, tests, tool_xml_tree) assert "No valid test(s) found." in lint_ctx.warn_messages assert "Test 1: Cannot specify outputs in a test expecting failure." in lint_ctx.error_messages assert ( @@ -1708,7 +1701,7 @@ def test_tests_expect_failure_output(lint_ctx): def test_tests_without_expectations(lint_ctx): tool_xml_tree = get_xml_tree(TESTS_WO_EXPECTATIONS) - run_lint(lint_ctx, tests.lint_tests, tool_xml_tree) + run_lint_module(lint_ctx, tests, tool_xml_tree) assert ( "Test 1: No outputs or expectations defined for tests, this test is likely invalid." in lint_ctx.warn_messages ) @@ -1721,7 +1714,7 @@ def test_tests_without_expectations(lint_ctx): def test_tests_valid(lint_ctx): tool_xml_tree = get_xml_tree(TESTS_VALID) - run_lint(lint_ctx, tests.lint_tests, tool_xml_tree) + run_lint_module(lint_ctx, tests, tool_xml_tree) assert "1 test(s) found." in lint_ctx.valid_messages assert not lint_ctx.info_messages assert len(lint_ctx.valid_messages) == 1 @@ -1731,7 +1724,7 @@ def test_tests_valid(lint_ctx): def test_tests_asserts(lint_ctx): tool_xml_tree = get_xml_tree(ASSERTS) - run_lint(lint_ctx, tests.lint_tests, tool_xml_tree) + run_lint_module(lint_ctx, tests, tool_xml_tree) assert "Test 1: unknown assertion 'invalid'" in lint_ctx.error_messages assert "Test 1: unknown attribute 'invalid_attrib' for 'has_text'" in lint_ctx.error_messages assert "Test 1: missing attribute 'text' for 'has_text'" in lint_ctx.error_messages @@ -1749,7 +1742,7 @@ def test_tests_asserts(lint_ctx): def test_tests_output_type_mismatch(lint_ctx): tool_xml_tree = get_xml_tree(TESTS_OUTPUT_TYPE_MISMATCH) - run_lint(lint_ctx, tests.lint_tests, tool_xml_tree) + run_lint_module(lint_ctx, tests, tool_xml_tree) assert ( "Test 1: test output collection_name does not correspond to a 'data' output, but a 'collection'" in lint_ctx.error_messages @@ -1764,7 +1757,7 @@ def test_tests_output_type_mismatch(lint_ctx): def test_tests_discover_outputs(lint_ctx): tool_xml_tree = get_xml_tree(TESTS_DISCOVER_OUTPUTS) - run_lint(lint_ctx, tests.lint_tests, tool_xml_tree) + run_lint_module(lint_ctx, tests, tool_xml_tree) assert ( "Test 3: test output 'data_name' must have a 'count' attribute and/or 'discovered_dataset' children" in lint_ctx.error_messages @@ -1787,7 +1780,7 @@ def test_tests_discover_outputs(lint_ctx): def test_tests_expect_num_outputs_filter(lint_ctx): tool_xml_tree = get_xml_tree(TESTS_EXPECT_NUM_OUTPUTS_FILTER) - run_lint(lint_ctx, tests.lint_tests, tool_xml_tree) + run_lint_module(lint_ctx, tests, tool_xml_tree) assert "Test should specify 'expect_num_outputs' if outputs have filters" in lint_ctx.warn_messages assert len(lint_ctx.warn_messages) == 1 assert len(lint_ctx.error_messages) == 0 @@ -1795,7 +1788,7 @@ def test_tests_expect_num_outputs_filter(lint_ctx): def test_tests_compare_attrib_incompatibility(lint_ctx): tool_xml_tree = get_xml_tree(TESTS_COMPARE_ATTRIB_INCOMPATIBILITY) - run_lint(lint_ctx, tests.lint_tests, tool_xml_tree) + run_lint_module(lint_ctx, tests, tool_xml_tree) assert 'Test 1: Attribute decompress is incompatible with compare="re_match".' in lint_ctx.error_messages assert 'Test 1: Attribute sort is incompatible with compare="contains".' in lint_ctx.error_messages assert not lint_ctx.info_messages @@ -1806,7 +1799,7 @@ def test_tests_compare_attrib_incompatibility(lint_ctx): def test_xml_order(lint_ctx): tool_xml_tree = get_xml_tree(XML_ORDER) - run_lint(lint_ctx, xml_order.lint_xml_order, tool_xml_tree) + run_lint_module(lint_ctx, xml_order, tool_xml_tree) assert "Unknown tag [wrong_tag] encountered, this may result in a warning in the future." in lint_ctx.info_messages assert "Best practice violation [stdio] elements should come before [command]" in lint_ctx.warn_messages assert len(lint_ctx.info_messages) == 1 From dd2a3887852af5b8127a0650695cad7d0f9adee7 Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Sun, 26 Nov 2023 13:57:52 +0100 Subject: [PATCH 04/32] split command linter --- lib/galaxy/tool_util/linters/command.py | 134 +++++++++++++++-------- test/unit/tool_util/test_tool_linters.py | 44 ++++---- 2 files changed, 110 insertions(+), 68 deletions(-) diff --git a/lib/galaxy/tool_util/linters/command.py b/lib/galaxy/tool_util/linters/command.py index 9d2dadd1b844..be2c0338a5ed 100644 --- a/lib/galaxy/tool_util/linters/command.py +++ b/lib/galaxy/tool_util/linters/command.py @@ -1,54 +1,92 @@ -"""This module contains a linting function for a tool's command description. +"""This module contains linters for a tool's command description. A command description describes how to build the command-line to execute from supplied inputs. """ +from typing import TYPE_CHECKING +from galaxy.tool_util.lint import Linter -def lint_command(tool_xml, lint_ctx): - """Ensure tool contains exactly one command and check attributes.""" - root = tool_xml.find("./command") - if root is None: - root = tool_xml.getroot() - - commands = tool_xml.findall("./command") - if len(commands) > 1: - lint_ctx.error("More than one command tag found, behavior undefined.", node=commands[1]) - return - - if len(commands) == 0: - lint_ctx.error("No command tag found, must specify a command template to execute.", node=root) - return - - command = get_command(tool_xml) - if command.text is None: - lint_ctx.error("Command is empty.", node=root) - elif "TODO" in command.text: - lint_ctx.warn("Command template contains TODO text.", node=command) - - command_attrib = command.attrib - interpreter_type = None - for key, value in command_attrib.items(): - if key == "interpreter": - interpreter_type = value - elif key == "detect_errors": - detect_errors = value - if detect_errors not in ["default", "exit_code", "aggressive"]: - lint_ctx.warn(f"Unknown detect_errors attribute [{detect_errors}]", node=command) - - interpreter_info = "" - if interpreter_type: - interpreter_info = f" with interpreter of type [{interpreter_type}]" - if interpreter_type: - lint_ctx.warn("Command uses deprecated 'interpreter' attribute.", node=command) - lint_ctx.info(f"Tool contains a command{interpreter_info}.", node=command) - - -def get_command(tool_xml): - """Get command XML element from supplied XML root.""" - root = tool_xml.getroot() - commands = root.findall("command") - command = None - if len(commands) == 1: - command = commands[0] - return command +if TYPE_CHECKING: + from galaxy.tool_util.lint import LintContext + from galaxy.tool_util.parser.interface import ToolSource + + +class CommandMultiple(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + commands = tool_xml.findall("./command") + if len(commands) > 1: + lint_ctx.error("More than one command tag found, behavior undefined.", node=commands[1]) + + +class CommandMissing(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("./command") + if root is None: + root = tool_xml.getroot() + command = tool_xml.find("./command") + if command is None: + lint_ctx.error("No command tag found, must specify a command template to execute.", node=root) + + +class CommandEmpty(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("./command") + if root is None: + root = tool_xml.getroot() + command = tool_xml.find("./command") + if command is not None and command.text is None: + lint_ctx.error("Command is empty.", node=root) + + +class CommandTODO(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + command = tool_xml.find("./command") + if command is not None and command.text is not None and "TODO" in command.text: + lint_ctx.warn("Command template contains TODO text.", node=command) + + +class CommandInterpreterDeprecated(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + command = tool_xml.find("./command") + if command is None: + return + interpreter_type = command.attrib.get("interpreter", None) + if interpreter_type is not None: + lint_ctx.warn("Command uses deprecated 'interpreter' attribute.", node=command) + + +class CommandInfo(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + command = tool_xml.find("./command") + if command is None: + return + interpreter_type = command.attrib.get("interpreter", None) + interpreter_info = "" + if interpreter_type: + interpreter_info = f" with interpreter of type [{interpreter_type}]" + lint_ctx.info(f"Tool contains a command{interpreter_info}.", node=command) diff --git a/test/unit/tool_util/test_tool_linters.py b/test/unit/tool_util/test_tool_linters.py index a9be82463c26..3676cb335819 100644 --- a/test/unit/tool_util/test_tool_linters.py +++ b/test/unit/tool_util/test_tool_linters.py @@ -62,8 +62,8 @@ # tests tool xml for command linter COMMAND_MULTIPLE = """ - - + ls + df """ COMMAND_MISSING = """ @@ -967,35 +967,39 @@ def test_citations_valid(lint_ctx): def test_command_multiple(lint_ctx): - tool_xml_tree = get_xml_tree(COMMAND_MULTIPLE) - run_lint_module(lint_ctx, command, tool_xml_tree) - assert "More than one command tag found, behavior undefined." in lint_ctx.error_messages - assert len(lint_ctx.error_messages) == 1 - assert not lint_ctx.info_messages + tool_source = get_xml_tool_source(COMMAND_MULTIPLE) + run_lint_module(lint_ctx, command, tool_source) + assert lint_ctx.error_messages == ["More than one command tag found, behavior undefined."] + assert lint_ctx.info_messages == ["Tool contains a command."] assert not lint_ctx.valid_messages assert not lint_ctx.warn_messages def test_command_missing(lint_ctx): - tool_xml_tree = get_xml_tree(COMMAND_MISSING) - run_lint_module(lint_ctx, command, tool_xml_tree) - assert "No command tag found, must specify a command template to execute." in lint_ctx.error_messages + tool_source = get_xml_tool_source(COMMAND_MISSING) + run_lint_module(lint_ctx, command, tool_source) + assert lint_ctx.error_messages == ["No command tag found, must specify a command template to execute."] + assert not lint_ctx.info_messages + assert not lint_ctx.valid_messages + assert not lint_ctx.warn_messages def test_command_todo(lint_ctx): - tool_xml_tree = get_xml_tree(COMMAND_TODO) - run_lint_module(lint_ctx, command, tool_xml_tree) - assert "Tool contains a command." in lint_ctx.info_messages - assert "Command template contains TODO text." in lint_ctx.warn_messages + tool_source = get_xml_tool_source(COMMAND_TODO) + run_lint_module(lint_ctx, command, tool_source) + assert not lint_ctx.error_messages + assert lint_ctx.info_messages == ["Tool contains a command."] + assert lint_ctx.warn_messages == ["Command template contains TODO text."] + assert not lint_ctx.valid_messages def test_command_detect_errors_interpreter(lint_ctx): - tool_xml_tree = get_xml_tree(COMMAND_DETECT_ERRORS_INTERPRETER) - run_lint_module(lint_ctx, command, tool_xml_tree) - assert "Command uses deprecated 'interpreter' attribute." in lint_ctx.warn_messages - assert "Tool contains a command with interpreter of type [python]." in lint_ctx.info_messages - assert "Unknown detect_errors attribute [nonsense]" in lint_ctx.warn_messages - assert "Command is empty." in lint_ctx.error_messages + tool_source = get_xml_tool_source(COMMAND_DETECT_ERRORS_INTERPRETER) + run_lint_module(lint_ctx, command, tool_source) + assert lint_ctx.error_messages == ["Command is empty."] + assert lint_ctx.warn_messages == ["Command uses deprecated 'interpreter' attribute."] + assert lint_ctx.info_messages == ["Tool contains a command with interpreter of type [python]."] + assert not lint_ctx.valid_messages def test_general_missing_tool_id_name_version(lint_ctx): From 7b3d38c6f9d2f45f207466af89bddfc8817e4bf0 Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Sun, 26 Nov 2023 14:11:05 +0100 Subject: [PATCH 05/32] make xml_order a Linter --- lib/galaxy/tool_util/linters/xml_order.py | 41 ++++++++++++++--------- test/unit/tool_util/test_tool_linters.py | 10 +++--- 2 files changed, 29 insertions(+), 22 deletions(-) diff --git a/lib/galaxy/tool_util/linters/xml_order.py b/lib/galaxy/tool_util/linters/xml_order.py index 7fb2bd261331..82d17f4e5ac7 100644 --- a/lib/galaxy/tool_util/linters/xml_order.py +++ b/lib/galaxy/tool_util/linters/xml_order.py @@ -1,8 +1,15 @@ -"""This module contains a linting functions for tool XML block order. +"""This module contains a linter for tool XML block order. For more information on the IUC standard for XML block order see - https://github.com/galaxy-iuc/standards. """ +from typing import TYPE_CHECKING + +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 # https://github.com/galaxy-iuc/standards # https://github.com/galaxy-iuc/standards/pull/7/files @@ -43,21 +50,25 @@ ] -# Ensure the XML blocks appear in the correct order prescribed -# by the tool author best practices. -def lint_xml_order(tool_xml, lint_ctx): - tool_root = tool_xml.getroot() +class XMLOrder(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + tool_root = tool_xml.getroot() - if tool_root.attrib.get("tool_type", "") == "data_source": - tag_ordering = DATASOURCE_TAG_ORDER - else: - tag_ordering = TAG_ORDER + if tool_root.attrib.get("tool_type", "") == "data_source": + tag_ordering = DATASOURCE_TAG_ORDER + else: + tag_ordering = TAG_ORDER - last_tag = None - last_key = None - for elem in tool_root: - tag = elem.tag - if tag in tag_ordering: + last_tag = None + last_key = None + for elem in tool_root: + tag = elem.tag + if tag not in tag_ordering: + continue key = tag_ordering.index(tag) if last_key: if last_key > key: @@ -66,5 +77,3 @@ def lint_xml_order(tool_xml, lint_ctx): ) last_tag = tag last_key = key - else: - lint_ctx.info(f"Unknown tag [{tag}] encountered, this may result in a warning in the future.", node=elem) diff --git a/test/unit/tool_util/test_tool_linters.py b/test/unit/tool_util/test_tool_linters.py index 3676cb335819..9cbaa02424d8 100644 --- a/test/unit/tool_util/test_tool_linters.py +++ b/test/unit/tool_util/test_tool_linters.py @@ -1802,13 +1802,11 @@ def test_tests_compare_attrib_incompatibility(lint_ctx): def test_xml_order(lint_ctx): - tool_xml_tree = get_xml_tree(XML_ORDER) - run_lint_module(lint_ctx, xml_order, tool_xml_tree) - assert "Unknown tag [wrong_tag] encountered, this may result in a warning in the future." in lint_ctx.info_messages - assert "Best practice violation [stdio] elements should come before [command]" in lint_ctx.warn_messages - assert len(lint_ctx.info_messages) == 1 + tool_source = get_xml_tool_source(XML_ORDER) + run_lint_module(lint_ctx, xml_order, tool_source) + assert lint_ctx.warn_messages == ["Best practice violation [stdio] elements should come before [command]"] + assert not lint_ctx.info_messages assert not lint_ctx.valid_messages - assert len(lint_ctx.warn_messages) == 1 assert not lint_ctx.error_messages From 350a4394c81951ada52c93f3dc9b02ead0c2e960 Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Sun, 26 Nov 2023 14:45:02 +0100 Subject: [PATCH 06/32] split help linters --- lib/galaxy/tool_util/linters/help.py | 140 +++++++++++++++++------ test/unit/tool_util/test_tool_linters.py | 25 ++-- 2 files changed, 120 insertions(+), 45 deletions(-) diff --git a/lib/galaxy/tool_util/linters/help.py b/lib/galaxy/tool_util/linters/help.py index 0008d465662e..8c2260bc9925 100644 --- a/lib/galaxy/tool_util/linters/help.py +++ b/lib/galaxy/tool_util/linters/help.py @@ -1,43 +1,119 @@ """This module contains a linting function for a tool's help.""" +from typing import ( + TYPE_CHECKING, + Union, +) -from typing import Union - +from galaxy.tool_util.lint import Linter from galaxy.util import ( rst_to_html, unicodify, ) +if TYPE_CHECKING: + from galaxy.tool_util.lint import LintContext + from galaxy.tool_util.parser.interface import ToolSource + + +class HelpMultiple(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + helps = tool_xml.findall("./help") + if len(helps) > 1: + lint_ctx.error("More than one help section found, behavior undefined.", node=helps[1]) + + +class HelpMissing(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("./help") + if root is None: + root = tool_xml.getroot() + help = tool_xml.find("./help") + if help is None: + lint_ctx.warn("No help section found, consider adding a help section to your tool.", node=root) + + +class HelpEmpty(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + help = tool_xml.find("./help") + if help is None: + return + help_text = help.text or "" + if not help_text.strip(): + lint_ctx.warn("Help section appears to be empty.", node=help) + + +class HelpPresent(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + help = tool_xml.find("./help") + if help is None: + return + help_text = help.text or "" + if help_text.strip(): + lint_ctx.valid("Tool contains help section.", node=help) + + +class HelpTODO(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + help = tool_xml.find("./help") + if help is None: + return + help_text = help.text or "" + if "TODO" in help_text: + lint_ctx.warn("Help contains TODO text.", node=help) + + +class HelpInvalidRST(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + help = tool_xml.find("./help") + if help is None: + return + help_text = help.text or "" + if not help_text.strip(): + return + invalid_rst = rst_invalid(help_text) + if invalid_rst: + lint_ctx.warn(f"Invalid reStructuredText found in help - [{invalid_rst}].", node=help) + -def lint_help(tool_xml, lint_ctx): - """Ensure tool contains exactly one valid RST help block.""" - # determine node to report for general problems with help - root = tool_xml.find("./help") - if root is None: - root = tool_xml.getroot() - helps = tool_xml.findall("./help") - if len(helps) > 1: - lint_ctx.error("More than one help section found, behavior undefined.", node=helps[1]) - return - - if len(helps) == 0: - lint_ctx.warn("No help section found, consider adding a help section to your tool.", node=root) - return - - help_text = helps[0].text or "" - if not help_text.strip(): - lint_ctx.warn("Help section appears to be empty.", node=helps[0]) - return - - lint_ctx.valid("Tool contains help section.", node=helps[0]) - - if "TODO" in help_text: - lint_ctx.warn("Help contains TODO text.", node=helps[0]) - - invalid_rst = rst_invalid(help_text) - if invalid_rst: - lint_ctx.warn(f"Invalid reStructuredText found in help - [{invalid_rst}].", node=helps[0]) - else: - lint_ctx.valid("Help contains valid reStructuredText.", node=helps[0]) +class HelpValidRST(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + help = tool_xml.find("./help") + if help is None: + return + help_text = help.text or "" + if not help_text.strip(): + return + invalid_rst = rst_invalid(help_text) + if not invalid_rst: + lint_ctx.valid("Help contains valid reStructuredText.", node=help) def rst_invalid(text: str) -> Union[bool, str]: diff --git a/test/unit/tool_util/test_tool_linters.py b/test/unit/tool_util/test_tool_linters.py index 9cbaa02424d8..937e2fd5fe61 100644 --- a/test/unit/tool_util/test_tool_linters.py +++ b/test/unit/tool_util/test_tool_linters.py @@ -1063,18 +1063,17 @@ def test_general_valid_new_profile_fmt(lint_ctx): def test_help_multiple(lint_ctx): - tool_xml_tree = get_xml_tree(HELP_MULTIPLE) - run_lint_module(lint_ctx, help, tool_xml_tree) - assert "More than one help section found, behavior undefined." in lint_ctx.error_messages + tool_source = get_xml_tool_source(HELP_MULTIPLE) + run_lint_module(lint_ctx, help, tool_source) assert not lint_ctx.info_messages - assert not lint_ctx.valid_messages + assert len(lint_ctx.valid_messages) == 2 # has help and valid rst assert not lint_ctx.warn_messages - assert len(lint_ctx.error_messages) == 1 + assert lint_ctx.error_messages == ["More than one help section found, behavior undefined."] def test_help_absent(lint_ctx): - tool_xml_tree = get_xml_tree(HELP_ABSENT) - run_lint_module(lint_ctx, help, tool_xml_tree) + tool_source = get_xml_tool_source(HELP_ABSENT) + run_lint_module(lint_ctx, help, tool_source) assert "No help section found, consider adding a help section to your tool." in lint_ctx.warn_messages assert not lint_ctx.info_messages assert not lint_ctx.valid_messages @@ -1083,8 +1082,8 @@ def test_help_absent(lint_ctx): def test_help_empty(lint_ctx): - tool_xml_tree = get_xml_tree(HELP_EMPTY) - run_lint_module(lint_ctx, help, tool_xml_tree) + tool_source = get_xml_tool_source(HELP_EMPTY) + run_lint_module(lint_ctx, help, tool_source) assert "Help section appears to be empty." in lint_ctx.warn_messages assert not lint_ctx.info_messages assert not lint_ctx.valid_messages @@ -1093,8 +1092,8 @@ def test_help_empty(lint_ctx): def test_help_todo(lint_ctx): - tool_xml_tree = get_xml_tree(HELP_TODO) - run_lint_module(lint_ctx, help, tool_xml_tree) + tool_source = get_xml_tool_source(HELP_TODO) + run_lint_module(lint_ctx, help, tool_source) assert "Tool contains help section." in lint_ctx.valid_messages assert "Help contains valid reStructuredText." in lint_ctx.valid_messages assert "Help contains TODO text." in lint_ctx.warn_messages @@ -1105,8 +1104,8 @@ def test_help_todo(lint_ctx): def test_help_invalid_rst(lint_ctx): - tool_xml_tree = get_xml_tree(HELP_INVALID_RST) - run_lint_module(lint_ctx, help, tool_xml_tree) + tool_source = get_xml_tool_source(HELP_INVALID_RST) + run_lint_module(lint_ctx, help, tool_source) assert "Tool contains help section." in lint_ctx.valid_messages assert ( "Invalid reStructuredText found in help - [:2: (WARNING/2) Inline strong start-string without end-string.\n]." From bf340a52d755672a711413cf2081f53ab5b45c5f Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Fri, 1 Dec 2023 10:16:50 +0100 Subject: [PATCH 07/32] started with test linters --- lib/galaxy/tool_util/lint.py | 2 +- lib/galaxy/tool_util/linters/tests.py | 62 +++++++++++++++++++++--- test/unit/tool_util/test_tool_linters.py | 44 ++++++++--------- 3 files changed, 79 insertions(+), 29 deletions(-) diff --git a/lib/galaxy/tool_util/lint.py b/lib/galaxy/tool_util/lint.py index f5cf0988c3e4..d21cf38d5bab 100644 --- a/lib/galaxy/tool_util/lint.py +++ b/lib/galaxy/tool_util/lint.py @@ -100,7 +100,7 @@ class LintMessage: a message from the linter """ - def __init__(self, level: str, message: str): + def __init__(self, level: str, message: str, **kwargs): self.level = level self.message = message diff --git a/lib/galaxy/tool_util/linters/tests.py b/lib/galaxy/tool_util/linters/tests.py index 5f26dc64c602..b81c1940f0c9 100644 --- a/lib/galaxy/tool_util/linters/tests.py +++ b/lib/galaxy/tool_util/linters/tests.py @@ -1,15 +1,69 @@ """This module contains a linting functions for tool tests.""" -import typing from inspect import ( Parameter, signature, ) +from typing import TYPE_CHECKING, Union +from galaxy.tool_util.lint import Linter from galaxy.util import asbool from ._util import is_datasource from ..verify import asserts +if TYPE_CHECKING: + from galaxy.tool_util.lint import LintContext + from galaxy.tool_util.parser.interface import ToolSource + +lint_tool_types = ["default", "data_source", "manage_data"] + + +class TestsMissing(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + tests = tool_xml.findall("./tests/test") + root = tool_xml.find("./tests") or tool_xml.getroot() + if len(tests) == 0 and not is_datasource(tool_xml): + lint_ctx.warn("No tests found, most tools should define test cases.", node=root) + + +class TestsMissingDatasource(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + tests = tool_xml.findall("./tests/test") + root = tool_xml.find("./tests") or tool_xml.getroot() + if len(tests) == 0 and is_datasource(tool_xml): + lint_ctx.info("No tests found, that should be OK for data_sources.", node=root) + + +# TEST_ASSERT_TAGS = ("assert_stdout", "assert_stderr", "assert_command") +# class TestsHas(Linter): +# @classmethod +# def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): +# tool_xml = getattr(tool_source, "xml_tree", None) +# if not tool_xml: +# return +# tests = tool_xml.findall("./tests/test") +# root = tool_xml.find("./tests") or tool_xml.getroot() + +# for test_idx, test in enumerate(tests, start=1): +# valid = False +# if len(set(test.attrib) & set(("expect_failure", "expect_exit_code", "expect_num_outputs"))): +# valid = True +# for ta in TEST_ASSERT_TAGS: +# if test.findall(ta) is not None: +# valid = True +# break +# has_output_test = test.find("output") is not None or test.find("output_collection") is not None + + + def check_compare_attribs(element, lint_ctx, test_idx): COMPARE_COMPATIBILITY = { "sort": ["diff", "re_match", "re_match_multiline"], @@ -34,10 +88,6 @@ def lint_tests(tool_xml, lint_ctx): general_node = tool_xml.getroot() datasource = is_datasource(tool_xml) if not tests: - if not datasource: - lint_ctx.warn("No tests found, most tools should define test cases.", node=general_node) - elif datasource: - lint_ctx.info("No tests found, that should be OK for data_sources.", node=general_node) return num_valid_tests = 0 @@ -209,7 +259,7 @@ def _check_asserts(test_idx, assertions, lint_ctx): def _handle_optionals(annotation): as_dict = annotation.__dict__ - if "__origin__" in as_dict and as_dict["__origin__"] == typing.Union: + if "__origin__" in as_dict and as_dict["__origin__"] == Union: return as_dict["__args__"][0] return annotation diff --git a/test/unit/tool_util/test_tool_linters.py b/test/unit/tool_util/test_tool_linters.py index 937e2fd5fe61..3fc36c624384 100644 --- a/test/unit/tool_util/test_tool_linters.py +++ b/test/unit/tool_util/test_tool_linters.py @@ -1646,8 +1646,8 @@ def test_stdio_invalid_match(lint_ctx): def test_tests_absent(lint_ctx): - tool_xml_tree = get_xml_tree(TESTS_ABSENT) - run_lint_module(lint_ctx, tests, tool_xml_tree) + tool_source = get_xml_tool_source(TESTS_ABSENT) + run_lint_module(lint_ctx, tests, tool_source) assert "No tests found, most tools should define test cases." in lint_ctx.warn_messages assert not lint_ctx.info_messages assert not lint_ctx.valid_messages @@ -1656,8 +1656,8 @@ def test_tests_absent(lint_ctx): def test_tests_data_source(lint_ctx): - tool_xml_tree = get_xml_tree(TESTS_ABSENT_DATA_SOURCE) - run_lint_module(lint_ctx, tests, tool_xml_tree) + tool_source = get_xml_tool_source(TESTS_ABSENT_DATA_SOURCE) + run_lint_module(lint_ctx, tests, tool_source) assert "No tests found, that should be OK for data_sources." in lint_ctx.info_messages assert len(lint_ctx.info_messages) == 1 assert not lint_ctx.valid_messages @@ -1666,8 +1666,8 @@ def test_tests_data_source(lint_ctx): def test_tests_param_output_names(lint_ctx): - tool_xml_tree = get_xml_tree(TESTS_PARAM_OUTPUT_NAMES) - run_lint_module(lint_ctx, tests, tool_xml_tree) + tool_source = get_xml_tool_source(TESTS_PARAM_OUTPUT_NAMES) + run_lint_module(lint_ctx, tests, tool_source) assert "1 test(s) found." in lint_ctx.valid_messages assert "Test 1: Found test param tag without a name defined." in lint_ctx.error_messages assert "Test 1: Test param non_existent_test_name not found in the inputs" in lint_ctx.error_messages @@ -1688,8 +1688,8 @@ def test_tests_param_output_names(lint_ctx): def test_tests_expect_failure_output(lint_ctx): - tool_xml_tree = get_xml_tree(TESTS_EXPECT_FAILURE_OUTPUT) - run_lint_module(lint_ctx, tests, tool_xml_tree) + tool_source = get_xml_tool_source(TESTS_EXPECT_FAILURE_OUTPUT) + run_lint_module(lint_ctx, tests, tool_source) assert "No valid test(s) found." in lint_ctx.warn_messages assert "Test 1: Cannot specify outputs in a test expecting failure." in lint_ctx.error_messages assert ( @@ -1703,8 +1703,8 @@ def test_tests_expect_failure_output(lint_ctx): def test_tests_without_expectations(lint_ctx): - tool_xml_tree = get_xml_tree(TESTS_WO_EXPECTATIONS) - run_lint_module(lint_ctx, tests, tool_xml_tree) + tool_source = get_xml_tool_source(TESTS_WO_EXPECTATIONS) + run_lint_module(lint_ctx, tests, tool_source) assert ( "Test 1: No outputs or expectations defined for tests, this test is likely invalid." in lint_ctx.warn_messages ) @@ -1716,8 +1716,8 @@ def test_tests_without_expectations(lint_ctx): def test_tests_valid(lint_ctx): - tool_xml_tree = get_xml_tree(TESTS_VALID) - run_lint_module(lint_ctx, tests, tool_xml_tree) + tool_source = get_xml_tool_source(TESTS_VALID) + run_lint_module(lint_ctx, tests, tool_source) assert "1 test(s) found." in lint_ctx.valid_messages assert not lint_ctx.info_messages assert len(lint_ctx.valid_messages) == 1 @@ -1726,8 +1726,8 @@ def test_tests_valid(lint_ctx): def test_tests_asserts(lint_ctx): - tool_xml_tree = get_xml_tree(ASSERTS) - run_lint_module(lint_ctx, tests, tool_xml_tree) + tool_source = get_xml_tool_source(ASSERTS) + run_lint_module(lint_ctx, tests, tool_source) assert "Test 1: unknown assertion 'invalid'" in lint_ctx.error_messages assert "Test 1: unknown attribute 'invalid_attrib' for 'has_text'" in lint_ctx.error_messages assert "Test 1: missing attribute 'text' for 'has_text'" in lint_ctx.error_messages @@ -1744,8 +1744,8 @@ def test_tests_asserts(lint_ctx): def test_tests_output_type_mismatch(lint_ctx): - tool_xml_tree = get_xml_tree(TESTS_OUTPUT_TYPE_MISMATCH) - run_lint_module(lint_ctx, tests, tool_xml_tree) + tool_source = get_xml_tool_source(TESTS_OUTPUT_TYPE_MISMATCH) + run_lint_module(lint_ctx, tests, tool_source) assert ( "Test 1: test output collection_name does not correspond to a 'data' output, but a 'collection'" in lint_ctx.error_messages @@ -1759,8 +1759,8 @@ def test_tests_output_type_mismatch(lint_ctx): def test_tests_discover_outputs(lint_ctx): - tool_xml_tree = get_xml_tree(TESTS_DISCOVER_OUTPUTS) - run_lint_module(lint_ctx, tests, tool_xml_tree) + tool_source = get_xml_tool_source(TESTS_DISCOVER_OUTPUTS) + run_lint_module(lint_ctx, tests, tool_source) assert ( "Test 3: test output 'data_name' must have a 'count' attribute and/or 'discovered_dataset' children" in lint_ctx.error_messages @@ -1782,16 +1782,16 @@ def test_tests_discover_outputs(lint_ctx): def test_tests_expect_num_outputs_filter(lint_ctx): - tool_xml_tree = get_xml_tree(TESTS_EXPECT_NUM_OUTPUTS_FILTER) - run_lint_module(lint_ctx, tests, tool_xml_tree) + tool_source = get_xml_tool_source(TESTS_EXPECT_NUM_OUTPUTS_FILTER) + run_lint_module(lint_ctx, tests, tool_source) assert "Test should specify 'expect_num_outputs' if outputs have filters" in lint_ctx.warn_messages assert len(lint_ctx.warn_messages) == 1 assert len(lint_ctx.error_messages) == 0 def test_tests_compare_attrib_incompatibility(lint_ctx): - tool_xml_tree = get_xml_tree(TESTS_COMPARE_ATTRIB_INCOMPATIBILITY) - run_lint_module(lint_ctx, tests, tool_xml_tree) + tool_source = get_xml_tool_source(TESTS_COMPARE_ATTRIB_INCOMPATIBILITY) + run_lint_module(lint_ctx, tests, tool_source) assert 'Test 1: Attribute decompress is incompatible with compare="re_match".' in lint_ctx.error_messages assert 'Test 1: Attribute sort is incompatible with compare="contains".' in lint_ctx.error_messages assert not lint_ctx.info_messages From 3bccffc552bde1fb685744eca445bf16aeac3a10 Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Mon, 4 Dec 2023 17:19:17 +0100 Subject: [PATCH 08/32] split test linters --- lib/galaxy/tool_util/linters/tests.py | 688 ++++++++++++++++------- test/unit/tool_util/test_tool_linters.py | 4 +- 2 files changed, 491 insertions(+), 201 deletions(-) diff --git a/lib/galaxy/tool_util/linters/tests.py b/lib/galaxy/tool_util/linters/tests.py index b81c1940f0c9..0d7f83fd0292 100644 --- a/lib/galaxy/tool_util/linters/tests.py +++ b/lib/galaxy/tool_util/linters/tests.py @@ -3,17 +3,21 @@ Parameter, signature, ) -from typing import TYPE_CHECKING, Union +from typing import ( + List, + Optional, + TYPE_CHECKING, +) from galaxy.tool_util.lint import Linter from galaxy.util import asbool from ._util import is_datasource from ..verify import asserts - if TYPE_CHECKING: from galaxy.tool_util.lint import LintContext from galaxy.tool_util.parser.interface import ToolSource + from galaxy.util.etree import Element lint_tool_types = ["default", "data_source", "manage_data"] @@ -25,7 +29,9 @@ def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): if not tool_xml: return tests = tool_xml.findall("./tests/test") - root = tool_xml.find("./tests") or tool_xml.getroot() + root = tool_xml.find("./tests") + if root is None: + root = tool_xml.getroot() if len(tests) == 0 and not is_datasource(tool_xml): lint_ctx.warn("No tests found, most tools should define test cases.", node=root) @@ -37,231 +43,515 @@ def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): if not tool_xml: return tests = tool_xml.findall("./tests/test") - root = tool_xml.find("./tests") or tool_xml.getroot() + root = tool_xml.find("./tests") + if root is None: + root = tool_xml.getroot() if len(tests) == 0 and is_datasource(tool_xml): lint_ctx.info("No tests found, that should be OK for data_sources.", node=root) -# TEST_ASSERT_TAGS = ("assert_stdout", "assert_stderr", "assert_command") -# class TestsHas(Linter): -# @classmethod -# def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): -# tool_xml = getattr(tool_source, "xml_tree", None) -# if not tool_xml: -# return -# tests = tool_xml.findall("./tests/test") -# root = tool_xml.find("./tests") or tool_xml.getroot() - -# for test_idx, test in enumerate(tests, start=1): -# valid = False -# if len(set(test.attrib) & set(("expect_failure", "expect_exit_code", "expect_num_outputs"))): -# valid = True -# for ta in TEST_ASSERT_TAGS: -# if test.findall(ta) is not None: -# valid = True -# break -# has_output_test = test.find("output") is not None or test.find("output_collection") is not None - - - -def check_compare_attribs(element, lint_ctx, test_idx): - COMPARE_COMPATIBILITY = { - "sort": ["diff", "re_match", "re_match_multiline"], - "lines_diff": ["diff", "re_match", "contains"], - "decompress": ["diff"], - "delta": ["sim_size"], - "delta_frac": ["sim_size"], - } - compare = element.get("compare", "diff") - for attrib in COMPARE_COMPATIBILITY: - if attrib in element.attrib and compare not in COMPARE_COMPATIBILITY[attrib]: - lint_ctx.error( - f'Test {test_idx}: Attribute {attrib} is incompatible with compare="{compare}".', node=element - ) - - -def lint_tests(tool_xml, lint_ctx): - # determine node to report for general problems with tests - tests = tool_xml.findall("./tests/test") - general_node = tool_xml.find("./tests") - if general_node is None: - general_node = tool_xml.getroot() - datasource = is_datasource(tool_xml) - if not tests: - return - - num_valid_tests = 0 - for test_idx, test in enumerate(tests, start=1): - has_test = False - test_expect = ("expect_failure", "expect_exit_code", "expect_num_outputs") - for te in test_expect: - if te in test.attrib: - has_test = True - break - test_assert = ("assert_stdout", "assert_stderr", "assert_command") - for ta in test_assert: - assertions = test.findall(ta) - if len(assertions) == 0: - continue - if len(assertions) > 1: - lint_ctx.error(f"Test {test_idx}: More than one {ta} found. Only the first is considered.", node=test) - has_test = True - _check_asserts(test_idx, assertions, lint_ctx) - _check_asserts(test_idx, test.findall(".//assert_contents"), lint_ctx) - - # check if expect_num_outputs is set if there are outputs with filters - # (except for tests with expect_failure .. which can't have test outputs) - filter = tool_xml.findall("./outputs//filter") - if len(filter) > 0 and "expect_num_outputs" not in test.attrib: - if not asbool(test.attrib.get("expect_failure", False)): - lint_ctx.warn("Test should specify 'expect_num_outputs' if outputs have filters", node=test) +class TestsAssertsMultiple(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + tests = tool_xml.findall("./tests/test") + for test_idx, test in enumerate(tests, start=1): + # TODO same would be nice also for assert_contents + for ta in ("assert_stdout", "assert_stderr", "assert_command"): + if len(test.findall(ta)) > 1: + lint_ctx.error( + f"Test {test_idx}: More than one {ta} found. Only the first is considered.", node=test + ) - # really simple test that test parameters are also present in the inputs - for param in test.findall("param"): - name = param.attrib.get("name", None) - if not name: - lint_ctx.error(f"Test {test_idx}: Found test param tag without a name defined.", node=param) - continue - name = name.split("|")[-1] - xpaths = [f"@name='{name}'", f"@argument='{name}'", f"@argument='-{name}'", f"@argument='--{name}'"] - if "_" in name: - xpaths += [f"@argument='-{name.replace('_', '-')}'", f"@argument='--{name.replace('_', '-')}'"] - found = False - for xp in xpaths: - inxpath = f".//inputs//param[{xp}]" - inparam = tool_xml.findall(inxpath) - if len(inparam) > 0: - found = True - break - if not found: - lint_ctx.error(f"Test {test_idx}: Test param {name} not found in the inputs", node=param) - output_data_or_collection = _collect_output_names(tool_xml) - found_output_test = False - for output in test.findall("output") + test.findall("output_collection"): - found_output_test = True - name = output.attrib.get("name", None) - if not name: - lint_ctx.error(f"Test {test_idx}: Found {output.tag} tag without a name defined.", node=output) - continue - if name not in output_data_or_collection: - lint_ctx.error( - f"Test {test_idx}: Found {output.tag} tag with unknown name [{name}], valid names {list(output_data_or_collection)}", - node=output, - ) - continue +class TestsAssertsUnknown(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + tests = tool_xml.findall("./tests/test") + for test_idx, test in enumerate(tests, start=1): + for a in test.xpath( + ".//*[self::assert_contents or self::assert_stdout or self::assert_stderr or self::assert_command]//*" + ): + assert_function_name = "assert_" + a.tag + if assert_function_name not in asserts.assertion_functions: + lint_ctx.error(f"Test {test_idx}: unknown assertion '{a.tag}'", node=a) - if output.tag == "output": - check_compare_attribs(output, lint_ctx, test_idx) - elements = output.findall("./element") - if elements: - for element in elements: - check_compare_attribs(element, lint_ctx, test_idx) - - # check that - # - test/output corresponds to outputs/data and - # - test/collection to outputs/output_collection - corresponding_output = output_data_or_collection[name] - if output.tag == "output" and corresponding_output.tag != "data": - lint_ctx.error( - f"Test {test_idx}: test output {name} does not correspond to a 'data' output, but a '{corresponding_output.tag}'", - node=output, - ) - elif output.tag == "output_collection" and corresponding_output.tag != "collection": - lint_ctx.error( - f"Test {test_idx}: test collection output '{name}' does not correspond to a 'output_collection' output, but a '{corresponding_output.tag}'", - node=output, + +class TestsAssertsUnknownAttrib(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + tests = tool_xml.findall("./tests/test") + for test_idx, test in enumerate(tests, start=1): + for a in test.xpath( + ".//*[self::assert_contents or self::assert_stdout or self::assert_stderr or self::assert_command]//*" + ): + assert_function_name = "assert_" + a.tag + if assert_function_name not in asserts.assertion_functions: + continue + assert_function_sig = signature(asserts.assertion_functions[assert_function_name]) + # check of the attributes + for attrib in a.attrib: + if attrib not in assert_function_sig.parameters: + lint_ctx.error(f"Test {test_idx}: unknown attribute '{attrib}' for '{a.tag}'", node=a) + + +class TestsAssertsMissingAttrib(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + tests = tool_xml.findall("./tests/test") + for test_idx, test in enumerate(tests, start=1): + for a in test.xpath( + ".//*[self::assert_contents or self::assert_stdout or self::assert_stderr or self::assert_command]//*" + ): + assert_function_name = "assert_" + a.tag + if assert_function_name not in asserts.assertion_functions: + continue + assert_function_sig = signature(asserts.assertion_functions[assert_function_name]) + # check missing required attributes + for p in assert_function_sig.parameters: + if p in ["output", "output_bytes", "verify_assertions_function", "children"]: + continue + if assert_function_sig.parameters[p].default is Parameter.empty and p not in a.attrib: + lint_ctx.error(f"Test {test_idx}: missing attribute '{p}' for '{a.tag}'", node=a) + + +class TestsAssertsHasNQuant(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + tests = tool_xml.findall("./tests/test") + for test_idx, test in enumerate(tests, start=1): + for a in test.xpath( + ".//*[self::assert_contents or self::assert_stdout or self::assert_stderr or self::assert_command]//*" + ): + if a.tag not in ["has_n_lines", "has_n_columns"]: + continue + if not (set(a.attrib) & set(["n", "min", "max"])): + lint_ctx.error(f"Test {test_idx}: '{a.tag}' needs to specify 'n', 'min', or 'max'", node=a) + + +class TestsAssertsHasSizeQuant(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + tests = tool_xml.findall("./tests/test") + for test_idx, test in enumerate(tests, start=1): + for a in test.xpath( + ".//*[self::assert_contents or self::assert_stdout or self::assert_stderr or self::assert_command]//*" + ): + if a.tag != "has_size": + continue + if set(a.attrib) & set(["value", "min", "max"]): + lint_ctx.error(f"Test {test_idx}: '{a.tag}' needs to specify 'value', 'min', or 'max'", node=a) + + +class TestsExpectNumOutputs(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + tests = tool_xml.findall("./tests/test") + for test_idx, test in enumerate(tests, start=1): + # check if expect_num_outputs is set if there are outputs with filters + # (except for tests with expect_failure .. which can't have test outputs) + filter = tool_xml.find(".//filter") + if not ( + filter is None + or "expect_num_outputs" in test.attrib + or asbool(test.attrib.get("expect_failure", False)) + ): + lint_ctx.warn( + f"Test {test_idx}: should specify 'expect_num_outputs' if outputs have filters", node=test ) - # check that discovered data is tested sufficiently - discover_datasets = corresponding_output.find(".//discover_datasets") - if discover_datasets is not None: - if output.tag == "output": - if "count" not in output.attrib and output.find("./discovered_dataset") is None: + +class TestsParamName(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + tests = tool_xml.findall("./tests/test") + for test_idx, test in enumerate(tests, start=1): + for param in test.findall("param"): + if not param.attrib.get("name", None): + lint_ctx.error(f"Test {test_idx}: Found test param tag without a name defined.", node=param) + + +class TestsParamInInputs(Linter): + """ + really simple linter that test parameters are also present in the inputs + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + tests = tool_xml.findall("./tests/test") + for test_idx, test in enumerate(tests, start=1): + for param in test.findall("param"): + name = param.attrib.get("name", None) + if not name: + continue + name = name.split("|")[-1] + xpaths = [f"@name='{name}'", f"@argument='{name}'", f"@argument='-{name}'", f"@argument='--{name}'"] + if "_" in name: + xpaths += [f"@argument='-{name.replace('_', '-')}'", f"@argument='--{name.replace('_', '-')}'"] + found = False + for xp in xpaths: + inxpath = f".//inputs//param[{xp}]" + inparam = tool_xml.findall(inxpath) + if len(inparam) > 0: + found = True + break + if not found: + lint_ctx.error(f"Test {test_idx}: Test param {name} not found in the inputs", node=param) + + +class TestsOutputName(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + tests = tool_xml.findall("./tests/test") + for test_idx, test in enumerate(tests, start=1): + for output in test.findall("output") + test.findall("output_collection"): + if not output.attrib.get("name", None): + lint_ctx.error(f"Test {test_idx}: Found {output.tag} tag without a name defined.", node=output) + + +class TestsOutputDefined(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + output_data_or_collection = _collect_output_names(tool_xml) + tests = tool_xml.findall("./tests/test") + for test_idx, test in enumerate(tests, start=1): + for output in test.findall("output") + test.findall("output_collection"): + name = output.attrib.get("name", None) + if not name: + continue + if name not in output_data_or_collection: + lint_ctx.error( + f"Test {test_idx}: Found {output.tag} tag with unknown name [{name}], valid names {list(output_data_or_collection)}", + node=output, + ) + + +class TestsOutputCorresponding(Linter): + """ + Linter checking if test/output corresponds to outputs/data + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + output_data_or_collection = _collect_output_names(tool_xml) + tests = tool_xml.findall("./tests/test") + for test_idx, test in enumerate(tests, start=1): + for output in test.findall("output") + test.findall("output_collection"): + name = output.attrib.get("name", None) + if not name: + continue + if name not in output_data_or_collection: + continue + + # - test/collection to outputs/output_collection + corresponding_output = output_data_or_collection[name] + if output.tag == "output" and corresponding_output.tag != "data": + lint_ctx.error( + f"Test {test_idx}: test output {name} does not correspond to a 'data' output, but a '{corresponding_output.tag}'", + node=output, + ) + + +class TestsOutputCollectionCorresponding(Linter): + """ + Linter checking if test/collection corresponds to outputs/output_collection + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + output_data_or_collection = _collect_output_names(tool_xml) + tests = tool_xml.findall("./tests/test") + for test_idx, test in enumerate(tests, start=1): + for output in test.findall("output") + test.findall("output_collection"): + name = output.attrib.get("name", None) + if not name: + continue + if name not in output_data_or_collection: + continue + + # - test/collection to outputs/output_collection + corresponding_output = output_data_or_collection[name] + if output.tag == "output_collection" and corresponding_output.tag != "collection": + lint_ctx.error( + f"Test {test_idx}: test collection output '{name}' does not correspond to a 'output_collection' output, but a '{corresponding_output.tag}'", + node=output, + ) + + +class TestsOutputCompareAttrib(Linter): + """ + Linter checking compatibility of output attributes with the value + of the compare attribute + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + tests = tool_xml.findall("./tests/test") + COMPARE_COMPATIBILITY = { + "sort": ["diff", "re_match", "re_match_multiline"], + "lines_diff": ["diff", "re_match", "contains"], + "decompress": ["diff"], + "delta": ["sim_size"], + "delta_frac": ["sim_size"], + } + for test_idx, test in enumerate(tests, start=1): + for output in test.xpath(".//*[self::output or self::element or self::discovered_dataset]"): + compare = output.get("compare", "diff") + for attrib in COMPARE_COMPATIBILITY: + if attrib in output.attrib and compare not in COMPARE_COMPATIBILITY[attrib]: lint_ctx.error( - f"Test {test_idx}: test output '{name}' must have a 'count' attribute and/or 'discovered_dataset' children", + f'Test {test_idx}: Attribute {attrib} is incompatible with compare="{compare}".', node=output, ) - else: - if "count" not in output.attrib and len(elements) == 0: + + +class TestsOutputCheckDiscovered(Linter): + """ + Linter checking that discovered elements of outputs are tested with + a count attribute or listing some discovered_dataset + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + output_data_or_collection = _collect_output_names(tool_xml) + tests = tool_xml.findall("./tests/test") + for test_idx, test in enumerate(tests, start=1): + for output in test.findall("output"): + name = output.attrib.get("name", None) + if not name: + continue + if name not in output_data_or_collection: + continue + + # - test/collection to outputs/output_collection + corresponding_output = output_data_or_collection[name] + discover_datasets = corresponding_output.find(".//discover_datasets") + if discover_datasets is None: + continue + if "count" not in output.attrib and output.find("./discovered_dataset") is None: + lint_ctx.error( + f"Test {test_idx}: test output '{name}' must have a 'count' attribute and/or 'discovered_dataset' children", + node=output, + ) + + +class TestsOutputCollectionCheckDiscovered(Linter): + """ + Linter checking that discovered elements of output collections + are tested with a count attribute or listing some elements + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + output_data_or_collection = _collect_output_names(tool_xml) + tests = tool_xml.findall("./tests/test") + for test_idx, test in enumerate(tests, start=1): + for output in test.findall("output_collection"): + name = output.attrib.get("name", None) + if not name: + continue + if name not in output_data_or_collection: + continue + # - test/collection to outputs/output_collection + corresponding_output = output_data_or_collection[name] + discover_datasets = corresponding_output.find(".//discover_datasets") + if discover_datasets is None: + continue + if "count" not in output.attrib and output.find("./element") is None: + lint_ctx.error( + f"Test {test_idx}: test collection '{name}' must have a 'count' attribute or 'element' children", + node=output, + ) + + +class TestsOutputCollectionCheckDiscoveredNested(Linter): + """ """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + output_data_or_collection = _collect_output_names(tool_xml) + tests = tool_xml.findall("./tests/test") + for test_idx, test in enumerate(tests, start=1): + for output in test.findall("output_collection"): + name = output.attrib.get("name", None) + if not name: + continue + if name not in output_data_or_collection: + continue + + # - test/collection to outputs/output_collection + corresponding_output = output_data_or_collection[name] + if corresponding_output.find(".//discover_datasets") is None: + continue + if corresponding_output.get("type", "") in ["list:list", "list:paired"]: + nested_elements = output.find("./element/element") + element_with_count = output.find("./element[@count]") + if nested_elements is None and element_with_count is None: lint_ctx.error( - f"Test {test_idx}: test collection '{name}' must have a 'count' attribute or 'element' children", + f"Test {test_idx}: test collection '{name}' must contain nested 'element' tags and/or element children with a 'count' attribute", node=output, ) - if corresponding_output.get("type", "") in ["list:list", "list:paired"]: - nested_elements = output.find("./element/element") - element_with_count = output.find("./element[@count]") - if nested_elements is None and element_with_count is None: - lint_ctx.error( - f"Test {test_idx}: test collection '{name}' must contain nested 'element' tags and/or element childen with a 'count' attribute", - node=output, - ) - if asbool(test.attrib.get("expect_failure", False)): - if found_output_test: + +class TestsOutputFailing(Linter): + """ """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + tests = tool_xml.findall("./tests/test") + for test_idx, test in enumerate(tests, start=1): + if not asbool(test.attrib.get("expect_failure", False)): + continue + if test.find("output") is not None or test.find("output_collection") is not None: lint_ctx.error(f"Test {test_idx}: Cannot specify outputs in a test expecting failure.", node=test) + + +class TestsExpectNumOutputsFailing(Linter): + """ """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + tests = tool_xml.findall("./tests/test") + for test_idx, test in enumerate(tests, start=1): + if not asbool(test.attrib.get("expect_failure", False)): + continue + if test.find("output") is not None or test.find("output_collection") is not None: continue if "expect_num_outputs" in test.attrib: lint_ctx.error( f"Test {test_idx}: Cannot make assumptions on the number of outputs in a test expecting failure.", node=test, ) - continue - has_test = has_test or found_output_test - if not has_test: - lint_ctx.warn( - f"Test {test_idx}: No outputs or expectations defined for tests, this test is likely invalid.", - node=test, - ) - else: - num_valid_tests += 1 - if num_valid_tests or datasource: - lint_ctx.valid(f"{num_valid_tests} test(s) found.", node=general_node) - else: - lint_ctx.warn("No valid test(s) found.", node=general_node) +class TestsHasExpectations(Linter): + """ """ + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + tests = tool_xml.findall("./tests/test") + _check_and_count_valid(tests, lint_ctx) -def _check_asserts(test_idx, assertions, lint_ctx): - """ - assertions is a list of assert_contents, assert_stdout, assert_stderr, assert_command - in practice only for the first case the list may be longer than one - """ - for assertion in assertions: - for i, a in enumerate(assertion.iter()): - if i == 0: # skip root note itself - continue - assert_function_name = "assert_" + a.tag - if assert_function_name not in asserts.assertion_functions: - lint_ctx.error(f"Test {test_idx}: unknown assertion '{a.tag}'", node=a) - continue - assert_function_sig = signature(asserts.assertion_functions[assert_function_name]) - # check of the attributes - for attrib in a.attrib: - if attrib not in assert_function_sig.parameters: - lint_ctx.error(f"Test {test_idx}: unknown attribute '{attrib}' for '{a.tag}'", node=a) - continue - # check missing required attributes - for p in assert_function_sig.parameters: - if p in ["output", "output_bytes", "verify_assertions_function", "children"]: - continue - if assert_function_sig.parameters[p].default is Parameter.empty and p not in a.attrib: - lint_ctx.error(f"Test {test_idx}: missing attribute '{p}' for '{a.tag}'", node=a) - # has_n_lines, has_n_columns, and has_size need to specify n/value, min, or max - if a.tag in ["has_n_lines", "has_n_columns"]: - if "n" not in a.attrib and "min" not in a.attrib and "max" not in a.attrib: - lint_ctx.error(f"Test {test_idx}: '{a.tag}' needs to specify 'n', 'min', or 'max'", node=a) - if a.tag == "has_size": - if "value" not in a.attrib and "min" not in a.attrib and "max" not in a.attrib: - lint_ctx.error(f"Test {test_idx}: '{a.tag}' needs to specify 'value', 'min', or 'max'", node=a) + +class TestsNoValid(Linter): + """ """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + general_node = tool_xml.find("./tests") + if general_node is None: + general_node = tool_xml.getroot() + tests = tool_xml.findall("./tests/test") + if not tests: + return + num_valid_tests = _check_and_count_valid(tests, None) + if num_valid_tests or is_datasource(tool_xml): + lint_ctx.valid(f"{num_valid_tests} test(s) found.", node=general_node) -def _handle_optionals(annotation): - as_dict = annotation.__dict__ - if "__origin__" in as_dict and as_dict["__origin__"] == Union: - return as_dict["__args__"][0] - return annotation +class TestsValid(Linter): + """ """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + general_node = tool_xml.find("./tests") + if general_node is None: + general_node = tool_xml.getroot() + tests = tool_xml.findall("./tests/test") + if not tests: + return + num_valid_tests = _check_and_count_valid(tests, None) + if not (num_valid_tests or is_datasource(tool_xml)): + lint_ctx.warn("No valid test(s) found.", node=general_node) + + +def _check_and_count_valid(tests: List["Element"], lint_ctx: Optional["LintContext"] = None): + num_valid = 0 + for test_idx, test in enumerate(tests, start=1): + valid = False + valid |= bool(set(test.attrib) & set(("expect_failure", "expect_exit_code", "expect_num_outputs"))) + for ta in ("assert_stdout", "assert_stderr", "assert_command"): + if test.find(ta) is not None: + valid = True + found_output_test = test.find("output") is not None or test.find("output_collection") is not None + if asbool(test.attrib.get("expect_failure", False)): + if found_output_test or "expect_num_outputs" in test.attrib: + continue + valid |= found_output_test + if not valid: + if lint_ctx: + lint_ctx.warn( + f"Test {test_idx}: No outputs or expectations defined for tests, this test is likely invalid.", + node=test, + ) + else: + num_valid += 1 + return num_valid def _collect_output_names(tool_xml): diff --git a/test/unit/tool_util/test_tool_linters.py b/test/unit/tool_util/test_tool_linters.py index 3fc36c624384..d1a1a190c3b9 100644 --- a/test/unit/tool_util/test_tool_linters.py +++ b/test/unit/tool_util/test_tool_linters.py @@ -1770,11 +1770,11 @@ def test_tests_discover_outputs(lint_ctx): in lint_ctx.error_messages ) assert ( - "Test 3: test collection 'collection_name' must contain nested 'element' tags and/or element childen with a 'count' attribute" + "Test 3: test collection 'collection_name' must contain nested 'element' tags and/or element children with a 'count' attribute" in lint_ctx.error_messages ) assert ( - "Test 5: test collection 'collection_name' must contain nested 'element' tags and/or element childen with a 'count' attribute" + "Test 5: test collection 'collection_name' must contain nested 'element' tags and/or element children with a 'count' attribute" in lint_ctx.error_messages ) assert not lint_ctx.warn_messages From 5ae6594fab64c637e18f68b1bd6f594f8391c8cb Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Mon, 4 Dec 2023 19:27:54 +0100 Subject: [PATCH 09/32] split output linters --- lib/galaxy/tool_util/linters/outputs.py | 267 +++++++++++++++-------- test/unit/tool_util/test_tool_linters.py | 22 +- 2 files changed, 192 insertions(+), 97 deletions(-) diff --git a/lib/galaxy/tool_util/linters/outputs.py b/lib/galaxy/tool_util/linters/outputs.py index e034b0fe606d..4fdce739e1f5 100644 --- a/lib/galaxy/tool_util/linters/outputs.py +++ b/lib/galaxy/tool_util/linters/outputs.py @@ -3,131 +3,226 @@ from packaging.version import Version -from galaxy.util import ( - etree, - string_as_bool, -) +from galaxy.tool_util.lint import Linter +from galaxy.util import string_as_bool from ._util import is_valid_cheetah_placeholder from ..parser.output_collection_def import NAMED_PATTERNS if TYPE_CHECKING: from galaxy.tool_util.lint import LintContext from galaxy.tool_util.parser import ToolSource + from galaxy.util.etree import Element + + +class OutputsMissing(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + tool_node = tool_xml.find("./outputs") + if tool_node is None: + tool_node = tool_xml.getroot() + if len(tool_xml.findall("./outputs")) == 0: + lint_ctx.warn("Tool contains no outputs section, most tools should produce outputs.", node=tool_node) + + +class OutputsMultiple(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + outputs = tool_xml.findall("./outputs") + if len(outputs) > 1: + lint_ctx.warn("Tool contains multiple output sections, behavior undefined.", node=outputs[1]) + + +class OutputsNameMissing(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for output in tool_xml.findall("./outputs/data") + tool_xml.findall("./outputs/collection"): + if "name" not in output.attrib: + # TODO make this an error if there is no discover_datasets / from_work_dir (is this then still a problem) + lint_ctx.warn("Tool output doesn't define a name - this is likely a problem.", node=output) + + +class OutputsNameInvalidCheetah(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for output in tool_xml.findall("./outputs/data[@name]") + tool_xml.findall("./outputs/collection[@name]"): + if not is_valid_cheetah_placeholder(output.attrib["name"]): + lint_ctx.warn( + f'Tool output name [{output.attrib["name"]}] is not a valid Cheetah placeholder.', node=output + ) -def lint_output(tool_source: "ToolSource", lint_ctx: "LintContext"): - """Check output elements, ensure there is at least one and check attributes.""" - tool_xml = getattr(tool_source, "xml_tree", None) - if tool_xml is None: - return - profile = tool_source.parse_profile() - - outputs = tool_xml.findall("./outputs") - # determine node to report for general problems with outputs - tool_node = tool_xml.find("./outputs") - if tool_node is None: - tool_node = tool_xml.getroot() - if len(outputs) == 0: - lint_ctx.warn("Tool contains no outputs section, most tools should produce outputs.", node=tool_node) - return - if len(outputs) > 1: - lint_ctx.warn("Tool contains multiple output sections, behavior undefined.", node=outputs[1]) - num_outputs = 0 - labels = set() - names = set() - for output in list(outputs[0]): - if output.tag is etree.Comment: - continue - if output.tag not in ["data", "collection"]: - lint_ctx.warn(f"Unknown element found in outputs [{output.tag}]", node=output) - continue - num_outputs += 1 - if "name" not in output.attrib: - lint_ctx.warn("Tool output doesn't define a name - this is likely a problem.", node=output) - # TODO make this an error if there is no discover_datasets / from_work_dir (is this then still a problem) - elif not is_valid_cheetah_placeholder(output.attrib["name"]): - lint_ctx.warn( - f'Tool output name [{output.attrib["name"]}] is not a valid Cheetah placeholder.', node=output - ) - - name = output.attrib.get("name") - if name is not None: +class OutputsNameDuplicated(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + names = set() + for output in tool_xml.findall("./outputs/data[@name]") + tool_xml.findall("./outputs/collection[@name]"): + name = output.attrib["name"] if name in names: lint_ctx.error(f"Tool output [{name}] has duplicated name", node=output) names.add(name) - label = output.attrib.get("label", "${tool.name} on ${on_string}") - if label in labels: - filter_node = output.find(".//filter") - if filter_node is not None: + +class OutputsLabelDuplicatedFilter(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + labels = set() + for output in tool_xml.findall("./outputs/data") + tool_xml.findall("./outputs/collection"): + name = output.attrib.get("name", "") + label = output.attrib.get("label", "${tool.name} on ${on_string}") + if label in labels and output.find(".//filter") is not None: lint_ctx.warn( f"Tool output [{name}] uses duplicated label '{label}', double check if filters imply disjoint cases", node=output, ) - else: + labels.add(label) + + +class OutputsLabelDuplicatedNoFilter(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + labels = set() + for output in tool_xml.findall("./outputs/data[@name]") + tool_xml.findall("./outputs/collection[@name]"): + name = output.attrib.get("name", "") + label = output.attrib.get("label", "${tool.name} on ${on_string}") + if label in labels and output.find(".//filter") is None: lint_ctx.warn(f"Tool output [{name}] uses duplicated label '{label}'", node=output) - labels.add(label) + labels.add(label) - format_set = False - if __check_format(output, lint_ctx, profile): - format_set = True - if output.tag == "data": - if "auto_format" in output.attrib and output.attrib["auto_format"]: - format_set = True - elif output.tag == "collection": +class OutputsCollectionType(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for output in tool_xml.findall("./outputs/collection"): if "type" not in output.attrib: lint_ctx.warn("Collection output with undefined 'type' found.", node=output) - if "structured_like" in output.attrib and "inherit_format" in output.attrib: - format_set = True - for sub in output: - if __check_pattern(sub): - format_set = True - elif __check_format(sub, lint_ctx, profile, allow_ext=True): + + +class OutputsNumber(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + outputs = tool_xml.findall("./outputs") + if len(outputs) == 0: + return + num_outputs = len(outputs[0].findall("./data")) + len(outputs[0].findall("./collection")) + lint_ctx.info(f"{num_outputs} outputs found.", node=outputs[0]) + + +class OutputsFormatInput(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + def _report(output: "Element"): + message = f"Using format='input' on {output.tag} is deprecated. Use the format_source attribute." + if Version(str(profile)) <= Version("16.01"): + lint_ctx.warn(message, node=output) + else: + lint_ctx.error(message, node=output) + + profile = tool_source.parse_profile() + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for output in tool_xml.findall("./outputs/data") + tool_xml.findall("./outputs/collection"): + fmt = output.attrib.get("format") + if fmt == "input": + _report(output) + for sub in output: + fmt = sub.attrib.get("format", sub.attrib.get("ext")) + if fmt == "input": + _report(output) + + +class OutputsFormat(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for output in tool_xml.findall("./outputs/data") + tool_xml.findall("./outputs/collection"): + format_set = False + if _check_format(output): format_set = True + if output.tag == "data": + if "auto_format" in output.attrib and output.attrib["auto_format"]: + format_set = True + + elif output.tag == "collection": + if "structured_like" in output.attrib and "inherit_format" in output.attrib: + format_set = True + for sub in output: + if _check_pattern(sub): + format_set = True + elif _check_format(sub): + format_set = True + + if not format_set: + lint_ctx.warn( + f"Tool {output.tag} output {output.attrib.get('name', 'with missing name')} doesn't define an output format.", + node=output, + ) - if not format_set: - lint_ctx.warn( - f"Tool {output.tag} output {output.attrib.get('name', 'with missing name')} doesn't define an output format.", - node=output, - ) - # TODO: check for different labels in case of multiple outputs - lint_ctx.info(f"{num_outputs} outputs found.", node=outputs[0]) +class OutputsFormatSourceIncomp(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + def _check_and_report(node): + if "format_source" in node.attrib and ("ext" in node.attrib or "format" in node.attrib): + lint_ctx.warn( + f"Tool {node.tag} output '{node.attrib.get('name', 'with missing name')}' should use either format_source or format/ext", + node=node, + ) + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for output in tool_xml.findall("./outputs/data") + tool_xml.findall("./outputs/collection"): + _check_and_report(output) + for sub in output: + _check_and_report(sub) -def __check_format(node, lint_ctx, profile: str, allow_ext=False): + +def _check_format(node): """ check if format/ext/format_source attribute is set in a given node issue a warning if the value is input return true (node defines format/ext) / false (else) """ - if "format_source" in node.attrib and ("ext" in node.attrib or "format" in node.attrib): - lint_ctx.warn( - f"Tool {node.tag} output '{node.attrib.get('name', 'with missing name')}' should use either format_source or format/ext", - node=node, - ) if "format_source" in node.attrib: return True if node.find(".//action[@type='format']") is not None: return True # if allowed (e.g. for discover_datasets), ext takes precedence over format - fmt = None - if allow_ext: - fmt = node.attrib.get("ext") - if fmt is None: - fmt = node.attrib.get("format") - if fmt == "input": - message = f"Using format='input' on {node.tag} is deprecated. Use the format_source attribute." - if Version(str(profile)) <= Version("16.01"): - lint_ctx.warn(message, node=node) - else: - lint_ctx.error(message, node=node) - + fmt = node.attrib.get("format", node.attrib.get("ext")) return fmt is not None -def __check_pattern(node): +def _check_pattern(node): """ check if - pattern attribute is set and defines the extension or diff --git a/test/unit/tool_util/test_tool_linters.py b/test/unit/tool_util/test_tool_linters.py index d1a1a190c3b9..1d4e1d173a56 100644 --- a/test/unit/tool_util/test_tool_linters.py +++ b/test/unit/tool_util/test_tool_linters.py @@ -1472,7 +1472,7 @@ def test_inputs_repeats(lint_ctx): def test_outputs_missing(lint_ctx): tool_source = get_xml_tool_source(OUTPUTS_MISSING) - run_lint(lint_ctx, outputs.lint_output, tool_source) + run_lint_module(lint_ctx, outputs, 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 @@ -1482,7 +1482,7 @@ def test_outputs_missing(lint_ctx): def test_outputs_multiple(lint_ctx): tool_source = get_xml_tool_source(OUTPUTS_MULTIPLE) - run_lint(lint_ctx, outputs.lint_output, tool_source) + run_lint_module(lint_ctx, outputs, tool_source) assert "0 outputs found." in lint_ctx.info_messages assert "Tool contains multiple output sections, behavior undefined." in lint_ctx.warn_messages assert len(lint_ctx.info_messages) == 1 @@ -1493,7 +1493,7 @@ def test_outputs_multiple(lint_ctx): def test_outputs_unknown_tag(lint_ctx): tool_source = get_xml_tool_source(OUTPUTS_UNKNOWN_TAG) - run_lint(lint_ctx, outputs.lint_output, tool_source) + run_lint_module(lint_ctx, outputs, tool_source) assert "0 outputs found." in lint_ctx.info_messages assert "Unknown element found in outputs [output]" in lint_ctx.warn_messages assert len(lint_ctx.info_messages) == 1 @@ -1504,7 +1504,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(lint_ctx, outputs.lint_output, tool_source) + run_lint_module(lint_ctx, outputs, tool_source) assert "2 outputs found." in lint_ctx.info_messages assert "Tool output doesn't define a name - this is likely a problem." in lint_ctx.warn_messages assert "Tool data output with missing name doesn't define an output format." in lint_ctx.warn_messages @@ -1519,7 +1519,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(lint_ctx, outputs.lint_output, tool_source) + run_lint_module(lint_ctx, outputs, 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 @@ -1530,7 +1530,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(lint_ctx, outputs.lint_output, tool_source) + run_lint_module(lint_ctx, outputs, 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 @@ -1541,7 +1541,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(lint_ctx, outputs.lint_output, tool_source) + run_lint_module(lint_ctx, outputs, 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 @@ -1551,7 +1551,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(lint_ctx, outputs.lint_output, tool_source) + run_lint_module(lint_ctx, outputs, tool_source) assert len(lint_ctx.info_messages) == 1 assert not lint_ctx.valid_messages assert not lint_ctx.warn_messages @@ -1560,7 +1560,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(lint_ctx, outputs.lint_output, tool_source) + run_lint_module(lint_ctx, outputs, tool_source) assert "1 outputs found." in lint_ctx.info_messages assert len(lint_ctx.info_messages) == 1 assert not lint_ctx.valid_messages @@ -1570,7 +1570,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(lint_ctx, outputs.lint_output, tool_source) + run_lint_module(lint_ctx, outputs, tool_source) assert "4 outputs found." in lint_ctx.info_messages assert len(lint_ctx.info_messages) == 1 assert not lint_ctx.valid_messages @@ -1784,7 +1784,7 @@ def test_tests_discover_outputs(lint_ctx): def test_tests_expect_num_outputs_filter(lint_ctx): tool_source = get_xml_tool_source(TESTS_EXPECT_NUM_OUTPUTS_FILTER) run_lint_module(lint_ctx, tests, tool_source) - assert "Test should specify 'expect_num_outputs' if outputs have filters" in lint_ctx.warn_messages + assert "Test 1: should specify 'expect_num_outputs' if outputs have filters" in lint_ctx.warn_messages assert len(lint_ctx.warn_messages) == 1 assert len(lint_ctx.error_messages) == 0 From d357ad0d3834365b8e303c9b9487ad4a5cb1b32d Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Tue, 5 Dec 2023 22:00:47 +0100 Subject: [PATCH 10/32] split input linters --- lib/galaxy/tool_util/linters/inputs.py | 1389 +++++++++++++++++----- test/unit/tool_util/test_tool_linters.py | 52 +- 2 files changed, 1141 insertions(+), 300 deletions(-) diff --git a/lib/galaxy/tool_util/linters/inputs.py b/lib/galaxy/tool_util/linters/inputs.py index 0c619e49095f..728e4368f657 100644 --- a/lib/galaxy/tool_util/linters/inputs.py +++ b/lib/galaxy/tool_util/linters/inputs.py @@ -1,8 +1,14 @@ """This module contains a linting functions for tool inputs.""" import ast import re -from typing import TYPE_CHECKING +from typing import ( + Iterator, + Optional, + Tuple, + TYPE_CHECKING, +) +from galaxy.tool_util.lint import Linter from galaxy.util import string_as_bool from ._util import ( is_datasource, @@ -13,6 +19,12 @@ if TYPE_CHECKING: from galaxy.tool_util.lint import LintContext from galaxy.tool_util.parser import ToolSource + from galaxy.util.etree import ( + Element, + ElementTree, + ) + +lint_tool_types = ["*"] FILTER_TYPES = [ "data_meta", @@ -125,298 +137,987 @@ ("./column", ["data_column"]), ] +# TODO lint for valid param type - attribute combinations +# TODO check if dataset is available for filters referring other datasets +# TODO check if ref input param is present for from_dataset + + +class InputsNum(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + tool_node = tool_xml.find("./inputs") + if tool_node is None: + tool_node = tool_xml.getroot() + num_inputs = len(tool_xml.findall("./inputs//param")) + if num_inputs: + lint_ctx.info(f"Found {num_inputs} input parameters.", node=tool_node) + -def lint_inputs(tool_source: "ToolSource", lint_ctx: "LintContext"): - """Lint parameters in a tool's inputs block.""" - tool_xml = getattr(tool_source, "xml_tree", None) - if tool_xml is None: - return - profile = tool_source.parse_profile() - datasource = is_datasource(tool_xml) - input_names = set() - inputs = tool_xml.findall("./inputs//param") - # determine node to report for general problems with outputs - tool_node = tool_xml.find("./inputs") - if tool_node is None: - tool_node = tool_xml.getroot() - num_inputs = 0 - for param in inputs: - num_inputs += 1 - param_attrib = param.attrib - if "name" not in param_attrib and "argument" not in param_attrib: - lint_ctx.error("Found param input with no name specified.", node=param) +class InputsMissing(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + tool_node = tool_xml.find("./inputs") + if tool_node is None: + tool_node = tool_xml.getroot() + num_inputs = len(tool_xml.findall("./inputs//param")) + if num_inputs == 0 and not is_datasource(tool_xml): + lint_ctx.warn("Found no input parameters.", node=tool_node) + + +class InputsMissingDataSource(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + tool_node = tool_xml.find("./inputs") + if tool_node is None: + tool_node = tool_xml.getroot() + num_inputs = len(tool_xml.findall("./inputs//param")) + if num_inputs == 0 and is_datasource(tool_xml): + lint_ctx.info("No input parameters, OK for data sources", node=tool_node) + + +class InputsDatasourceTags(Linter): + """ + Lint that datasource tools have display and uihints tags + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + tool_node = tool_xml.find("./inputs") + if tool_node is None: + tool_node = tool_xml.getroot() + inputs = tool_xml.findall("./inputs//param") + if is_datasource(tool_xml): + # TODO only display is subtag of inputs, uihints is a separate top level tag (supporting only attrib minwidth) + for datasource_tag in ("display", "uihints"): + if not any(param.tag == datasource_tag for param in inputs): + lint_ctx.info(f"{datasource_tag} tag usually present in data sources", node=tool_node) + + +def _iter_param(tool_xml: "ElementTree") -> Iterator[Tuple["Element", str]]: + for param in tool_xml.findall("./inputs//param"): + if "name" not in param.attrib and "argument" not in param.attrib: continue - param_name = _parse_name(param_attrib.get("name"), param_attrib.get("argument")) - if "name" in param_attrib and "argument" in param_attrib: - if param_attrib.get("name") == _parse_name(None, param_attrib.get("argument")): - lint_ctx.warn( - f"Param input [{param_name}] 'name' attribute is redundant if argument implies the same name.", - node=param, - ) - if param_name.strip() == "": - lint_ctx.error("Param input with empty name.", node=param) - elif not is_valid_cheetah_placeholder(param_name): - lint_ctx.warn(f"Param input [{param_name}] is not a valid Cheetah placeholder.", node=param) - - # check for parameters with duplicate names - path = [param_name] - parent = param - while True: - parent = parent.getparent() - if parent.tag == "inputs": - break - # parameters of the same name in different when branches are allowed - # just add the value of the when branch to the path (this also allows - # that the conditional select has the same name as params in the whens) - if parent.tag == "when": - path.append(str(parent.attrib.get("value"))) - else: - path.append(str(parent.attrib.get("name"))) - path_str = ".".join(reversed(path)) - if path_str in input_names: - lint_ctx.error(f"Tool defines multiple parameters with the same name: '{path_str}'", node=param) - input_names.add(path_str) - - if "type" not in param_attrib: - lint_ctx.error(f"Param input [{param_name}] input with no type specified.", node=param) + param_name = _parse_name(param.attrib.get("name"), param.attrib.get("argument")) + yield param, param_name + + +def _iter_param_type(tool_xml: "ElementTree") -> Iterator[Tuple["Element", str, str]]: + for param, param_name in _iter_param(tool_xml): + if "type" not in param.attrib: continue - elif param_attrib["type"].strip() == "": - lint_ctx.error(f"Param input [{param_name}] with empty type specified.", node=param) + param_type = param.attrib["type"] + if param_type == "": continue - param_type = param_attrib["type"] + yield param, param_name, param_type + + +class InputsName(Linter): + """ + Lint params define a name + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param in tool_xml.findall("./inputs//param"): + if "name" not in param.attrib and "argument" not in param.attrib: + lint_ctx.error("Found param input with no name specified.", node=param) + + +class InputsNameRedundantArgument(Linter): + """ + Lint params define a name + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param in tool_xml.findall("./inputs//param"): + if "name" in param.attrib and "argument" in param.attrib: + param_name = _parse_name(param.attrib.get("name"), param.attrib.get("argument")) + if param.attrib.get("name") == _parse_name(None, param.attrib.get("argument")): + lint_ctx.warn( + f"Param input [{param_name}] 'name' attribute is redundant if argument implies the same name.", + node=param, + ) + + +# TODO redundant with InputsNameValid +class InputsNameEmpty(Linter): + """ + Lint params define a non-empty name + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param, param_name in _iter_param(tool_xml): + if param_name.strip() == "": + lint_ctx.error("Param input with empty name.", node=param) + + +class InputsNameValid(Linter): + """ + Lint params define valid cheetah placeholder + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param, param_name in _iter_param(tool_xml): + if param_name != "" and not is_valid_cheetah_placeholder(param_name): + lint_ctx.warn(f"Param input [{param_name}] is not a valid Cheetah placeholder.", node=param) + - # TODO lint for valid param type - attribute combinations +# TODO xsd +class InputsType(Linter): + """ + Lint params define a type + """ - # lint for valid param type - child node combinations - for ptcc in PARAM_TYPE_CHILD_COMBINATIONS: - if param.find(ptcc[0]) is not None and param_type not in ptcc[1]: + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param, param_name in _iter_param(tool_xml): + if "type" not in param.attrib: + lint_ctx.error(f"Param input [{param_name}] input with no type specified.", node=param) + + +class InputsTypeEmpty(Linter): + """ + Lint params define a type + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param, param_name in _iter_param(tool_xml): + if "type" in param.attrib and param.attrib["type"] == "": + lint_ctx.error(f"Param input [{param_name}] with empty type specified.", node=param) + + +def _param_path(param: "Element", param_name: str) -> str: + path = [param_name] + parent = param + while True: + parent = parent.getparent() + if parent.tag == "inputs": + break + # parameters of the same name in different when branches are allowed + # just add the value of the when branch to the path (this also allows + # that the conditional select has the same name as params in the whens) + if parent.tag == "when": + path.append(str(parent.attrib.get("value"))) + else: + path.append(str(parent.attrib.get("name"))) + return ".".join(reversed(path)) + + +class InputsNameDuplicate(Linter): + """ + Lint params with duplicate names + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + input_names = set() + for param, param_name in _iter_param(tool_xml): + # check for parameters with duplicate names + path = _param_path(param, param_name) + if path in input_names: + lint_ctx.error(f"Tool defines multiple parameters with the same name: '{path}'", node=param) + input_names.add(path) + + +class InputsNameDuplicateOutput(Linter): + """ + Lint params names that are also used in outputs + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + input_names = set() + for param, param_name in _iter_param(tool_xml): + input_names.add(_param_path(param, param_name)) + + # check if there is an output with the same name as an input + outputs = tool_xml.findall("./outputs/*") + for output in outputs: + if output.get("name") in input_names: lint_ctx.error( - f"Parameter [{param_name}] '{ptcc[0]}' tags are only allowed for parameters of type {ptcc[1]}", - node=param, + f'Tool defines an output with a name equal to the name of an input: \'{output.get("name")}\'', + node=output, ) - # param type specific linting - if param_type == "data": - if "format" not in param_attrib: + +class InputsTypeChildCombination(Linter): + """ + Lint for invalid parameter type child combinations + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param, param_name, param_type in _iter_param_type(tool_xml): + # lint for valid param type - child node combinations + for ptcc in PARAM_TYPE_CHILD_COMBINATIONS: + if param.find(ptcc[0]) is not None and param_type not in ptcc[1]: + lint_ctx.error( + f"Parameter [{param_name}] '{ptcc[0]}' tags are only allowed for parameters of type {ptcc[1]}", + node=param, + ) + + +class InputsDataFormat(Linter): + """ + Lint for data params wo format + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param, param_name, param_type in _iter_param_type(tool_xml): + if param_type != "data": + continue + if "format" not in param.attrib: lint_ctx.warn( f"Param input [{param_name}] with no format specified - 'data' format will be assumed.", node=param ) + + +class InputsDataOptionsMultiple(Linter): + """ + Lint for data params with multiple options tags + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param, param_name, param_type in _iter_param_type(tool_xml): + if param_type != "data": + continue options = param.findall("./options") - has_options_filter_attribute = False - if len(options) == 1: - for oa in options[0].attrib: - if oa == "options_filter_attribute": - has_options_filter_attribute = True - else: - lint_ctx.error(f"Data parameter [{param_name}] uses invalid attribute: {oa}", node=param) - elif len(options) > 1: + if len(options) > 1: lint_ctx.error(f"Data parameter [{param_name}] contains multiple options elements.", node=options[1]) - # for data params only filters with key='build' of type='data_meta' are allowed - filters = param.findall("./options/filter") - for f in filters: - if not f.get("ref"): - lint_ctx.error( - f"Data parameter [{param_name}] filter needs to define a ref attribute", - node=f, - ) - if has_options_filter_attribute: - if f.get("type") != "data_meta": + + +class InputsDataOptionsAttrib(Linter): + """ + Lint for data params with options that have invalid attributes + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param, param_name, param_type in _iter_param_type(tool_xml): + if param_type != "data": + continue + options = param.find("./options") + if options is None: + continue + for oa in options.attrib: + if oa != "options_filter_attribute": + lint_ctx.error(f"Data parameter [{param_name}] uses invalid attribute: {oa}", node=param) + + +class InputsDataOptionsFilterAttribFiltersType(Linter): + """ + Lint for valid filter types for data parameters + if options set options_filter_attribute + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param, param_name, param_type in _iter_param_type(tool_xml): + if param_type != "data": + continue + options = param.find("./options") + if options is None: + continue + for filter in param.findall("./options/filter"): + if "options_filter_attribute" in options.attrib: + if filter.get("type") != "data_meta": lint_ctx.error( - f'Data parameter [{param_name}] for filters only type="data_meta" is allowed, found type="{f.get("type")}"', - node=f, + f'Data parameter [{param_name}] for filters only type="data_meta" is allowed, found type="{filter.get("type")}"', + node=filter, ) - else: - if f.get("key") != "dbkey" or f.get("type") != "data_meta": + + +class InputsDataOptionsFiltersType(Linter): + """ + Lint for valid filter types for data parameters + if options do not set options_filter_attribute + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param, param_name, param_type in _iter_param_type(tool_xml): + if param_type != "data": + continue + options = param.find("./options") + if options is None: + continue + for filter in param.findall("./options/filter"): + if "options_filter_attribute" not in options.attrib: + if filter.get("key") != "dbkey" or filter.get("type") != "data_meta": lint_ctx.error( - f'Data parameter [{param_name}] for filters only type="data_meta" and key="dbkey" are allowed, found type="{f.get("type")}" and key="{f.get("key")}"', - node=f, + f'Data parameter [{param_name}] for filters only type="data_meta" and key="dbkey" are allowed, found type="{filter.get("type")}" and key="{filter.get("key")}"', + node=filter, ) - elif param_type == "select": - # get dynamic/statically defined options - dynamic_options = param.get("dynamic_options", None) - options = param.findall("./options") - filters = param.findall("./options/filter") - select_options = param.findall("./option") +class InputsDataOptionsFiltersRef(Linter): + """ + Lint for set ref for filters of data parameters + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param, param_name, param_type in _iter_param_type(tool_xml): + if param_type != "data": + continue + options = param.find("./options") + if options is None: + continue + for filter in param.findall("./options/filter"): + if not filter.get("ref"): + lint_ctx.error( + f"Data parameter [{param_name}] filter needs to define a ref attribute", + node=filter, + ) + + +class InputsSelectDynamicOptions(Linter): + """ + Lint for select with deprecated dynamic_options attribute + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param, param_name, param_type in _iter_param_type(tool_xml): + if param_type != "select": + continue + dynamic_options = param.get("dynamic_options", None) if dynamic_options is not None: lint_ctx.warn( f"Select parameter [{param_name}] uses deprecated 'dynamic_options' attribute.", node=param ) - # check if options are defined by exactly one possibility + +class InputsSelectOptionsDef(Linter): + """ + Lint for valid ways to define select options + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param, param_name, param_type in _iter_param_type(tool_xml): + if param_type != "select": + continue + dynamic_options = param.get("dynamic_options", None) + options = param.findall("./options") + select_options = param.findall("./option") if param.getparent().tag != "conditional": if (dynamic_options is not None) + (len(options) > 0) + (len(select_options) > 0) != 1: lint_ctx.error( f"Select parameter [{param_name}] options have to be defined by either 'option' children elements, a 'options' element or the 'dynamic_options' attribute.", node=param, ) - else: - if len(select_options) == 0: - lint_ctx.error( - f"Select parameter of a conditional [{param_name}] options have to be defined by 'option' children elements.", - node=param, - ) - - # lint dynamic options - if len(options) == 1: - filters = options[0].findall("./filter") - # lint filters - # TODO check if dataset is available for filters referring other datasets - filter_adds_options = False - for f in filters: - ftype = f.get("type", None) - if ftype is None: - lint_ctx.error(f"Select parameter [{param_name}] contains filter without type.", node=f) - continue - if ftype not in FILTER_TYPES: - lint_ctx.error( - f"Select parameter [{param_name}] contains filter with unknown type '{ftype}'.", node=f - ) - continue - if ftype in ["add_value", "data_meta"]: - filter_adds_options = True - # TODO more linting of filters - - from_file = options[0].get("from_file", None) - from_parameter = options[0].get("from_parameter", None) - from_dataset = options[0].get("from_dataset", None) - from_data_table = options[0].get("from_data_table", None) - # TODO check if input param is present for from_dataset - if ( - from_file is None - and from_parameter is None - and from_dataset is None - and from_data_table is None - and not filter_adds_options - ): - lint_ctx.error( - f"Select parameter [{param_name}] options tag defines no options. Use 'from_dataset', 'from_data_table', or a filter that adds values.", - node=options[0], - ) - for deprecated_attr in ["from_file", "from_parameter", "options_filter_attribute", "transform_lines"]: - if options[0].get(deprecated_attr) is not None: - lint_ctx.warn( - f"Select parameter [{param_name}] options uses deprecated '{deprecated_attr}' attribute.", - node=options[0], - ) +class InputsSelectOptionsDefConditional(Linter): + """ + Lint for valid ways to define select options + (for a select in a conditional) + """ - if from_dataset is not None and from_data_table is not None: + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param, param_name, param_type in _iter_param_type(tool_xml): + if param_type != "select": + continue + dynamic_options = param.get("dynamic_options", None) + options = param.findall("./options") + select_options = param.findall("./option") + if param.getparent().tag == "conditional": + if len(select_options) == 0 or dynamic_options is not None or len(options) > 0: lint_ctx.error( - f"Select parameter [{param_name}] options uses 'from_dataset' and 'from_data_table' attribute.", - node=options[0], + f"Select parameter of a conditional [{param_name}] options have to be defined by 'option' children elements.", + node=param, ) - if options[0].get("meta_file_key", None) is not None and from_dataset is None: - lint_ctx.error( - f"Select parameter [{param_name}] 'meta_file_key' is only compatible with 'from_dataset'.", - node=options[0], - ) - elif len(options) > 1: - lint_ctx.error(f"Select parameter [{param_name}] contains multiple options elements.", node=options[1]) +# TODO xsd +class InputsSelectOptionValueMissing(Linter): + """ + Lint for select option tags without value + """ - # lint statically defined options + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param, param_name, param_type in _iter_param_type(tool_xml): + if param_type != "select": + continue + select_options = param.findall("./option") if any("value" not in option.attrib for option in select_options): lint_ctx.error(f"Select parameter [{param_name}] has option without value", node=param) + + +class InputsSelectOptionTextMissing(Linter): + """ + Lint for select option tags without value + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param, param_name, param_type in _iter_param_type(tool_xml): + if param_type != "select": + continue + select_options = param.findall("./option") if any(option.text is None for option in select_options): lint_ctx.warn(f"Select parameter [{param_name}] has option without text", node=param) - select_options_texts = list() + +class InputsSelectOptionDuplicateValue(Linter): + """ + Lint for select option with same value + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param, param_name, param_type in _iter_param_type(tool_xml): + if param_type != "select": + continue + select_options = param.findall("./option") select_options_values = list() for option in select_options: value = option.attrib.get("value", "") + select_options_values.append((value, option.attrib.get("selected", "false"))) + if len(set(select_options_values)) != len(select_options_values): + lint_ctx.error(f"Select parameter [{param_name}] has multiple options with the same value", node=param) + + +class InputsSelectOptionDuplicateText(Linter): + """ + Lint for select option with same text + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param, param_name, param_type in _iter_param_type(tool_xml): + if param_type != "select": + continue + select_options = param.findall("./option") + select_options_texts = list() + for option in select_options: if option.text is None: - text = value.capitalize() + text = option.attrib.get("value", "").capitalize() else: text = option.text select_options_texts.append((text, option.attrib.get("selected", "false"))) - select_options_values.append((value, option.attrib.get("selected", "false"))) if len(set(select_options_texts)) != len(select_options_texts): lint_ctx.error( f"Select parameter [{param_name}] has multiple options with the same text content", node=param ) - if len(set(select_options_values)) != len(select_options_values): - lint_ctx.error(f"Select parameter [{param_name}] has multiple options with the same value", node=param) - if param_type == "boolean": - truevalue = param_attrib.get("truevalue", "true") - falsevalue = param_attrib.get("falsevalue", "false") + +class InputsSelectOptionsMultiple(Linter): + """ + Lint dynamic options select for multiple options tags + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param, param_name, param_type in _iter_param_type(tool_xml): + if param_type != "select": + continue + options = param.findall("./options") + if len(options) > 1: + lint_ctx.error(f"Select parameter [{param_name}] contains multiple options elements.", node=options[1]) + + +# TODO xsd +class InputsSelectOptionsFilterTypeMissing(Linter): + """ + Lint dynamic options select for filters wo type + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param, param_name, param_type in _iter_param_type(tool_xml): + if param_type != "select": + continue + for filter in param.findall("./options/filter"): + ftype = filter.get("type", None) + if ftype is None: + lint_ctx.error(f"Select parameter [{param_name}] contains filter without type.", node=filter) + continue + + +class InputsSelectOptionsFilterValidType(Linter): + """ + Lint dynamic options select for filters with invalid type + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param, param_name, param_type in _iter_param_type(tool_xml): + if param_type != "select": + continue + for filter in param.findall("./options/filter"): + ftype = filter.get("type", None) + if ftype and ftype not in FILTER_TYPES: + lint_ctx.error( + f"Select parameter [{param_name}] contains filter with unknown type '{ftype}'.", node=filter + ) + + +class InputsSelectOptionsDefinesOptions(Linter): + """ + Lint dynamic options select for the potential to defile options + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param, param_name, param_type in _iter_param_type(tool_xml): + if param_type != "select": + continue + options = param.find("./options") + if options is None: + continue + filter_adds_options = any( + [f.get("type", None) in ["add_value", "data_meta"] for f in param.findall("./options/filter")] + ) + from_file = options.get("from_file", None) + from_parameter = options.get("from_parameter", None) + # TODO check if input param is present for from_dataset + from_dataset = options.get("from_dataset", None) + from_data_table = options.get("from_data_table", None) + + if ( + from_file is None + and from_parameter is None + and from_dataset is None + and from_data_table is None + and not filter_adds_options + ): + lint_ctx.error( + f"Select parameter [{param_name}] options tag defines no options. Use 'from_dataset', 'from_data_table', or a filter that adds values.", + node=options, + ) + + +class InputsSelectOptionsDeprecatedAttr(Linter): + """ + Lint dynamic options select deprecated attributes + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param, param_name, param_type in _iter_param_type(tool_xml): + if param_type != "select": + continue + options = param.find("./options") + if options is None: + continue + for deprecated_attr in ["from_file", "from_parameter", "options_filter_attribute", "transform_lines"]: + if options.get(deprecated_attr) is not None: + lint_ctx.warn( + f"Select parameter [{param_name}] options uses deprecated '{deprecated_attr}' attribute.", + node=options, + ) + + +class InputsSelectOptionsFromDatasetAndDatatable(Linter): + """ + Lint dynamic options select for from_dataset and from_data_table + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param, param_name, param_type in _iter_param_type(tool_xml): + if param_type != "select": + continue + options = param.find("./options") + if options is None: + continue + from_dataset = options.get("from_dataset", None) + from_data_table = options.get("from_data_table", None) + if from_dataset is not None and from_data_table is not None: + lint_ctx.error( + f"Select parameter [{param_name}] options uses 'from_dataset' and 'from_data_table' attribute.", + node=options, + ) + + +class InputsSelectOptionsMetaFileKey(Linter): + """ + Lint dynamic options select: meta_file_key attribute can only be used with from_dataset + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param, param_name, param_type in _iter_param_type(tool_xml): + if param_type != "select": + continue + options = param.find("./options") + if options is None: + continue + from_dataset = options.get("from_dataset", None) + if options.get("meta_file_key", None) is not None and from_dataset is None: + lint_ctx.error( + f"Select parameter [{param_name}] 'meta_file_key' is only compatible with 'from_dataset'.", + node=options, + ) + + +class InputsBoolDistinctValues(Linter): + """ + Lint booleans for distinct true/false value + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param, param_name, param_type in _iter_param_type(tool_xml): + if param_type != "boolean": + continue + profile = tool_source.parse_profile() + truevalue = param.attrib.get("truevalue", "true") + falsevalue = param.attrib.get("falsevalue", "false") problematic_booleans_allowed = profile < "23.1" lint_level = lint_ctx.warn if problematic_booleans_allowed else lint_ctx.error if truevalue == falsevalue: lint_level( f"Boolean parameter [{param_name}] needs distinct 'truevalue' and 'falsevalue' values.", node=param ) + + +class InputsBoolProblematic(Linter): + """ + Lint booleans for problematic values, i.e. truevalue being false and vice versa + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param, param_name, param_type in _iter_param_type(tool_xml): + if param_type != "boolean": + continue + profile = tool_source.parse_profile() + truevalue = param.attrib.get("truevalue", "true") + falsevalue = param.attrib.get("falsevalue", "false") + problematic_booleans_allowed = profile < "23.1" + lint_level = lint_ctx.warn if problematic_booleans_allowed else lint_ctx.error if truevalue.lower() == "false": lint_level(f"Boolean parameter [{param_name}] has invalid truevalue [{truevalue}].", node=param) if falsevalue.lower() == "true": lint_level(f"Boolean parameter [{param_name}] has invalid falsevalue [{falsevalue}].", node=param) - if param_type in ["select", "data_column", "drill_down"]: - multiple = string_as_bool(param_attrib.get("multiple", "false")) - optional = string_as_bool(param_attrib.get("optional", multiple)) - if param_attrib.get("display") == "checkboxes": + +class InputsSelectSingleCheckboxes(Linter): + """ + Lint selects that allow only a single selection but display as checkboxes + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param, param_name, param_type in _iter_param_type(tool_xml): + if param_type not in ["select", "data_column", "drill_down"]: + continue + multiple = string_as_bool(param.attrib.get("multiple", "false")) + if param.attrib.get("display") == "checkboxes": if not multiple: lint_ctx.error( f'Select [{param_name}] `display="checkboxes"` is incompatible with `multiple="false"`, remove the `display` attribute', node=param, ) + + +class InputsSelectMandatoryCheckboxes(Linter): + """ + Lint selects that are mandatory but display as checkboxes + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param, param_name, param_type in _iter_param_type(tool_xml): + if param_type not in ["select", "data_column", "drill_down"]: + continue + multiple = string_as_bool(param.attrib.get("multiple", "false")) + optional = string_as_bool(param.attrib.get("optional", multiple)) + if param.attrib.get("display") == "checkboxes": if not optional: lint_ctx.error( f'Select [{param_name}] `display="checkboxes"` is incompatible with `optional="false"`, remove the `display` attribute', node=param, ) - if param_attrib.get("display") == "radio": + + +class InputsSelectMultipleRadio(Linter): + """ + Lint selects that allow only multiple selections but display as radio + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param, param_name, param_type in _iter_param_type(tool_xml): + if param_type not in ["select", "data_column", "drill_down"]: + continue + multiple = string_as_bool(param.attrib.get("multiple", "false")) + if param.attrib.get("display") == "radio": if multiple: lint_ctx.error( f'Select [{param_name}] display="radio" is incompatible with multiple="true"', node=param ) + + +class InputsSelectOptionalRadio(Linter): + """ + Lint selects that are optional but display as radio + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param, param_name, param_type in _iter_param_type(tool_xml): + if param_type not in ["select", "data_column", "drill_down"]: + continue + multiple = string_as_bool(param.attrib.get("multiple", "false")) + optional = string_as_bool(param.attrib.get("optional", multiple)) + if param.attrib.get("display") == "radio": if optional: lint_ctx.error( f'Select [{param_name}] display="radio" is incompatible with optional="true"', node=param ) - # TODO: Validate type, much more... - # lint validators - # TODO check if dataset is available for validators referring other datasets - validators = param.findall("./validator") + +def _iter_param_validator(tool_xml: "ElementTree") -> Iterator[Tuple[str, str, "Element", str]]: + input_params = tool_xml.findall("./inputs//param[@type]") + for param in input_params: + try: + param_name = _parse_name(param.attrib.get("name"), param.attrib.get("argument")) + except ValueError: + continue + param_type = param.attrib["type"] + validators = param.findall("./validator[@type]") for validator in validators: vtype = validator.attrib["type"] + yield (param_name, param_type, validator, vtype) + + +class ValidatorParamIncompatible(Linter): + """ + Lint for validator type - parameter type incompatibilities + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param_name, param_type, validator, vtype in _iter_param_validator(tool_xml): if param_type in PARAMETER_VALIDATOR_TYPE_COMPATIBILITY: if vtype not in PARAMETER_VALIDATOR_TYPE_COMPATIBILITY[param_type]: lint_ctx.error( f"Parameter [{param_name}]: validator with an incompatible type '{vtype}'", node=validator ) + + +class ValidatorAttribIncompatible(Linter): + """ + Lint for incompatibilities between validator type and given attributes + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param_name, _, validator, vtype in _iter_param_validator(tool_xml): for attrib in ATTRIB_VALIDATOR_COMPATIBILITY: if attrib in validator.attrib and vtype not in ATTRIB_VALIDATOR_COMPATIBILITY[attrib]: lint_ctx.error( f"Parameter [{param_name}]: attribute '{attrib}' is incompatible with validator of type '{vtype}'", node=validator, ) + + +class ValidatorHasText(Linter): + """ + Lint that parameters that need text have text + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param_name, _, validator, vtype in _iter_param_validator(tool_xml): if vtype in ["expression", "regex"]: if validator.text is None: lint_ctx.error( f"Parameter [{param_name}]: {vtype} validators are expected to contain text", node=validator ) - else: - try: - if vtype == "regex": - re.compile(validator.text) - else: - ast.parse(validator.text, mode="eval") - except Exception as e: - lint_ctx.error( - f"Parameter [{param_name}]: '{validator.text}' is no valid regular expression: {str(e)}", - node=validator, - ) + + +class ValidatorHasNoText(Linter): + """ + Lint that parameters that need no text have no text + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param_name, _, validator, vtype in _iter_param_validator(tool_xml): if vtype not in ["expression", "regex"] and validator.text is not None: lint_ctx.warn( f"Parameter [{param_name}]: '{vtype}' validators are not expected to contain text (found '{validator.text}')", node=validator, ) + + +class ValidatorExpression(Linter): + """ + Lint that checks expressions / regexp + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param_name, _, validator, vtype in _iter_param_validator(tool_xml): + if vtype in ["expression", "regex"] and validator.text is not None: + try: + if vtype == "regex": + re.compile(validator.text) + else: + ast.parse(validator.text, mode="eval") + except Exception as e: + lint_ctx.error( + f"Parameter [{param_name}]: '{validator.text}' is no valid {vtype}: {str(e)}", + node=validator, + ) + + +class ValidatorMinMax(Linter): + """ + Lint for min/max for validator that need it + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param_name, _, validator, vtype in _iter_param_validator(tool_xml): if vtype in ["in_range", "length", "dataset_metadata_in_range"] and ( "min" not in validator.attrib and "max" not in validator.attrib ): @@ -424,6 +1125,19 @@ def lint_inputs(tool_source: "ToolSource", lint_ctx: "LintContext"): f"Parameter [{param_name}]: '{vtype}' validators need to define the 'min' or 'max' attribute(s)", node=validator, ) + + +class ValidatorDatasetMetadataEqualValue(Linter): + """ + Lint dataset_metadata_equal needs value or value_json + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param_name, _, validator, vtype in _iter_param_validator(tool_xml): if vtype in ["dataset_metadata_equal"]: if ( not ("value" in validator.attrib or "value_json" in validator.attrib) @@ -433,17 +1147,56 @@ def lint_inputs(tool_source: "ToolSource", lint_ctx: "LintContext"): f"Parameter [{param_name}]: '{vtype}' validators need to define the 'value'/'value_json' and 'metadata_name' attributes", node=validator, ) + + +class ValidatorDatasetMetadataEqualValueOrJson(Linter): + """ + Lint dataset_metadata_equal needs either value or value_json + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param_name, _, validator, vtype in _iter_param_validator(tool_xml): + if vtype in ["dataset_metadata_equal"]: if "value" in validator.attrib and "value_json" in validator.attrib: lint_ctx.error( f"Parameter [{param_name}]: '{vtype}' validators must not define the 'value' and the 'value_json' attributes", node=validator, ) + +class ValidatorMetadataCheckSkip(Linter): + """ + Lint metadata needs check or skip + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param_name, _, validator, vtype in _iter_param_validator(tool_xml): if vtype in ["metadata"] and ("check" not in validator.attrib and "skip" not in validator.attrib): lint_ctx.error( f"Parameter [{param_name}]: '{vtype}' validators need to define the 'check' or 'skip' attribute(s)", node=validator, ) + + +class ValidatorTableName(Linter): + """ + Lint table_name is present if needed + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param_name, _, validator, vtype in _iter_param_validator(tool_xml): if ( vtype in [ @@ -458,6 +1211,19 @@ def lint_inputs(tool_source: "ToolSource", lint_ctx: "LintContext"): f"Parameter [{param_name}]: '{vtype}' validators need to define the 'table_name' attribute", node=validator, ) + + +class ValidatorMetadataName(Linter): + """ + Lint metadata_name is present if needed + """ + + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for param_name, _, validator, vtype in _iter_param_validator(tool_xml): if ( vtype in [ @@ -473,117 +1239,192 @@ def lint_inputs(tool_source: "ToolSource", lint_ctx: "LintContext"): node=validator, ) - conditional_selects = tool_xml.findall("./inputs//conditional") - for conditional in conditional_selects: + +def _iter_conditional(tool_xml: "ElementTree") -> Iterator[Tuple["Element", Optional[str], "Element", Optional[str]]]: + conditionals = tool_xml.findall("./inputs//conditional") + for conditional in conditionals: conditional_name = conditional.get("name") - if not conditional_name: - lint_ctx.error("Conditional without a name", node=conditional) - if conditional.get("value_from"): - # Probably only the upload tool use this, no children elements + if conditional.get("value_from"): # Probably only the upload tool use this, no children elements continue - first_param = conditional.findall("param") - if len(first_param) != 1: - lint_ctx.error( - f"Conditional [{conditional_name}] needs exactly one child found {len(first_param)}", - node=conditional, - ) + first_param = conditional.find("param") + if first_param is None: continue - first_param = first_param[0] first_param_type = first_param.get("type") - if first_param_type == "boolean": - lint_ctx.warn( - f'Conditional [{conditional_name}] first param of type="boolean" is discouraged, use a select', - node=first_param, - ) - elif first_param_type != "select": - lint_ctx.error(f'Conditional [{conditional_name}] first param should have type="select"', node=first_param) - continue + yield conditional, conditional_name, first_param, first_param_type + - if first_param_type == "select": - select_options = _find_with_attribute(first_param, "option", "value") - option_ids = [option.get("value") for option in select_options] - else: # boolean - option_ids = [first_param.get("truevalue", "true"), first_param.get("falsevalue", "false")] +class ConditionalName(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + conditionals = tool_xml.findall("./inputs//conditional") + for conditional in conditionals: + conditional_name = conditional.get("name") + if not conditional_name: + lint_ctx.error("Conditional without a name", node=conditional) - for incomp in ["optional", "multiple"]: - if string_as_bool(first_param.get(incomp, False)): + +class ConditionalParamNumber(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + conditionals = tool_xml.findall("./inputs//conditional") + for conditional in conditionals: + conditional_name = conditional.get("name") + if conditional.get("value_from"): # Probably only the upload tool use this, no children elements + continue + first_param = conditional.findall("param") + if len(first_param) != 1: + lint_ctx.error( + f"Conditional [{conditional_name}] needs exactly one child found {len(first_param)}", + node=conditional, + ) + + +class ConditionalParamTypeBool(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for _, conditional_name, first_param, first_param_type in _iter_conditional(tool_xml): + if first_param_type == "boolean": lint_ctx.warn( - f'Conditional [{conditional_name}] test parameter cannot be {incomp}="true"', node=first_param + f'Conditional [{conditional_name}] first param of type="boolean" is discouraged, use a select', + node=first_param, ) - whens = conditional.findall("./when") - if any("value" not in when.attrib for when in whens): - lint_ctx.error(f"Conditional [{conditional_name}] when without value", node=conditional) - when_ids = [w.get("value") for w in whens if w.get("value") is not None] +class ConditionalParamType(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for _, conditional_name, first_param, first_param_type in _iter_conditional(tool_xml): + if first_param_type not in ["boolean", "select"]: + lint_ctx.error( + f'Conditional [{conditional_name}] first param should have type="select"', node=first_param + ) + - for option_id in option_ids: - if option_id not in when_ids: +class ConditionalParamIncompatibleAttributes(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for _, conditional_name, first_param, first_param_type in _iter_conditional(tool_xml): + if first_param_type not in ["boolean", "select"]: + continue + for incomp in ["optional", "multiple"]: + if string_as_bool(first_param.get(incomp, False)): + lint_ctx.warn( + f'Conditional [{conditional_name}] test parameter cannot be {incomp}="true"', node=first_param + ) + + +class ConditionalWhenValue(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for conditional, conditional_name, _, first_param_type in _iter_conditional(tool_xml): + if first_param_type not in ["boolean", "select"]: + continue + whens = conditional.findall("./when") + if any("value" not in when.attrib for when in whens): + lint_ctx.error(f"Conditional [{conditional_name}] when without value", node=conditional) + + +class ConditionalWhenMissing(Linter): + @classmethod + def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"): + tool_xml = getattr(tool_source, "xml_tree", None) + if not tool_xml: + return + for conditional, conditional_name, first_param, first_param_type in _iter_conditional(tool_xml): + if first_param_type not in ["boolean", "select"]: + continue + if first_param_type == "select": + options = first_param.findall("./option[@value]") + option_ids = set([option.get("value") for option in options]) + else: # boolean + option_ids = set([first_param.get("truevalue", "true"), first_param.get("falsevalue", "false")]) + whens = conditional.findall("./when[@value]") + when_ids = set([w.get("value") for w in whens if w.get("value")]) + for option_id in option_ids - when_ids: lint_ctx.warn( f"Conditional [{conditional_name}] no block found for {first_param_type} option '{option_id}'", node=conditional, ) - for when_id in when_ids: - if when_id not in option_ids: - if first_param_type == "select": - lint_ctx.warn( - f"Conditional [{conditional_name}] no