From 29f3f40296501d0637e919c62e6f19e2777fb09d Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Tue, 18 Jan 2022 20:08:41 +0100 Subject: [PATCH] add file name to linter message - add fname to LintMessage - LintContext make lint messages persistent over multiple linter calls before the messages were reset before the call of the next linter which is "impractical" if we want to use the lintmessages eg in planemo - add test for linting a tool + (internal and external) macros to ensure that the fname and line number is correct - add a simple linter test for yaml and json tools --- lib/galaxy/tool_util/lint.py | 39 +++-- lib/galaxy/tool_util/linters/_util.py | 9 ++ lib/galaxy/tool_util/linters/citations.py | 21 +-- lib/galaxy/tool_util/linters/command.py | 21 +-- lib/galaxy/tool_util/linters/cwl.py | 4 +- lib/galaxy/tool_util/linters/help.py | 23 ++- lib/galaxy/tool_util/linters/inputs.py | 135 ++++++++-------- lib/galaxy/tool_util/linters/outputs.py | 29 ++-- lib/galaxy/tool_util/linters/stdio.py | 24 ++- lib/galaxy/tool_util/linters/tests.py | 39 ++--- lib/galaxy/tool_util/linters/xml_order.py | 6 +- test/unit/tool_util/test_tool_linters.py | 189 +++++++++++++++++++++- 12 files changed, 353 insertions(+), 186 deletions(-) diff --git a/lib/galaxy/tool_util/lint.py b/lib/galaxy/tool_util/lint.py index aac34bb1cae..91435adfac1 100644 --- a/lib/galaxy/tool_util/lint.py +++ b/lib/galaxy/tool_util/lint.py @@ -64,19 +64,26 @@ def lint_xml_with(lint_context, tool_xml, extra_modules=None): class LintMessage: - def __init__(self, level, message, line, xpath=None): + """ + a message from the linter + """ + def __init__(self, level, message, line, fname, xpath=None): self.level = level self.message = message - self.line = line + self.line = line # 1 based + self.fname = fname self.xpath = xpath def __str__(self): rval = f".. {self.level.upper()}: {self.message}" if self.line is not None: - rval += f" (line {self.line})" + rval += " (" + if self.fname: + rval += f"{self.fname}:" + rval += str(self.line) + rval += ")" if self.xpath is not None: rval += f" [{self.xpath}]" - return rval @@ -91,12 +98,14 @@ class LintContext: self.object_name = object_name self.found_errors = False self.found_warns = False + self.message_list = [] def lint(self, name, lint_func, lint_target): name = name.replace("tsts", "tests")[len("lint_"):] if name in self.skip_types: return self.printed_linter_info = False + tmp_message_list = list(self.message_list) self.message_list = [] # call linter @@ -141,6 +150,7 @@ class LintContext: continue print_linter_info() print(f"{message}") + self.message_list = tmp_message_list + self.message_list @property def valid_messages(self): @@ -158,22 +168,23 @@ class LintContext: def error_messages(self): return [x.message for x in self.message_list if x.level == "error"] - def __handle_message(self, level, message, line, xpath, *args): + def __handle_message(self, level, message, line, fname, xpath, *args): if args: message = message % args - self.message_list.append(LintMessage(level=level, message=message, line=line, xpath=xpath)) + self.message_list.append(LintMessage(level=level, message=message, + line=line, fname=fname, xpath=xpath)) - def valid(self, message, line=None, xpath=None, *args): - self.__handle_message("check", message, line, xpath, *args) + def valid(self, message, line=None, fname=None, xpath=None, *args): + self.__handle_message("check", message, line, fname, xpath, *args) - def info(self, message, line=None, xpath=None, *args): - self.__handle_message("info", message, line, xpath, *args) + def info(self, message, line=None, fname=None, xpath=None, *args): + self.__handle_message("info", message, line, fname, xpath, *args) - def error(self, message, line=None, xpath=None, *args): - self.__handle_message("error", message, line, xpath, *args) + def error(self, message, line=None, fname=None, xpath=None, *args): + self.__handle_message("error", message, line, fname, xpath, *args) - def warn(self, message, line=None, xpath=None, *args): - self.__handle_message("warning", message, line, xpath, *args) + def warn(self, message, line=None, fname=None, xpath=None, *args): + self.__handle_message("warning", message, line, fname, xpath, *args) def failed(self, fail_level): found_warns = self.found_warns diff --git a/lib/galaxy/tool_util/linters/_util.py b/lib/galaxy/tool_util/linters/_util.py index 3224150c792..03e728b38ef 100644 --- a/lib/galaxy/tool_util/linters/_util.py +++ b/lib/galaxy/tool_util/linters/_util.py @@ -1,6 +1,15 @@ import re +def node_props(node, tool_xml): + if node is None: + return {"line": 0, "fname": None, "xpath": None} + else: + return {"line": node.sourceline, + "fname": node.base, + "xpath": tool_xml.getpath(node)} + + def is_datasource(tool_xml): """Returns true if the tool is a datasource tool""" return tool_xml.getroot().attrib.get('tool_type', '') == 'data_source' diff --git a/lib/galaxy/tool_util/linters/citations.py b/lib/galaxy/tool_util/linters/citations.py index 444fecf0650..6bab16702f3 100644 --- a/lib/galaxy/tool_util/linters/citations.py +++ b/lib/galaxy/tool_util/linters/citations.py @@ -3,41 +3,36 @@ Citations describe references that should be used when consumers of the tool publish results. """ +from ._util import node_props def lint_citations(tool_xml, lint_ctx): """Ensure tool contains at least one valid citation.""" root = tool_xml.getroot() - if root is not None: - root_line = root.sourceline - root_xpath = tool_xml.getpath(root) - else: - root_line = 1 - root_xpath = None citations = root.findall("citations") if len(citations) > 1: - lint_ctx.error("More than one citation section found, behavior undefined.", line=citations[1].sourceline, xpath=tool_xml.getpath(citations[1])) + lint_ctx.error("More than one citation section found, behavior undefined.", **node_props(citations[1], tool_xml)) return if len(citations) == 0: - lint_ctx.warn("No citations found, consider adding citations to your tool.", line=root_line, xpath=root_xpath) + lint_ctx.warn("No citations found, consider adding citations to your tool.", **node_props(root, tool_xml)) 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.", line=citation.sourceline, xpath=tool_xml.getpath(citation)) + lint_ctx.warn(f"Unknown tag discovered in citations block [{citation.tag}], will be ignored.", **node_props(citation, tool_xml)) 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.", line=citation.sourceline, xpath=tool_xml.getpath(citation)) + lint_ctx.warn(f"Unknown citation type discovered [{citation_type}], will be ignored.", **node_props(citation, tool_xml)) continue if citation.text is None or not citation.text.strip(): - lint_ctx.error(f'Empty {citation_type} citation.', line=citation.sourceline, xpath=tool_xml.getpath(citation)) + lint_ctx.error(f'Empty {citation_type} citation.', **node_props(citation, tool_xml)) continue valid_citations += 1 if valid_citations > 0: - lint_ctx.valid(f"Found {valid_citations} likely valid citations.", line=root_line, xpath=root_xpath) + lint_ctx.valid(f"Found {valid_citations} likely valid citations.", **node_props(root, tool_xml)) else: - lint_ctx.warn("Found no valid citations.", line=root_line, xpath=root_xpath) + lint_ctx.warn("Found no valid citations.", **node_props(root, tool_xml)) diff --git a/lib/galaxy/tool_util/linters/command.py b/lib/galaxy/tool_util/linters/command.py index 231cd6a5a0c..fa76d487e3a 100644 --- a/lib/galaxy/tool_util/linters/command.py +++ b/lib/galaxy/tool_util/linters/command.py @@ -3,31 +3,26 @@ A command description describes how to build the command-line to execute from supplied inputs. """ +from ._util import node_props def lint_command(tool_xml, lint_ctx): """Ensure tool contains exactly one command and check attributes.""" root = tool_xml.getroot() - if root is not None: - root_line = root.sourceline - root_path = tool_xml.getpath(root) - else: - root_line = 1 - root_path = None commands = root.findall("command") if len(commands) > 1: - lint_ctx.error("More than one command tag found, behavior undefined.", line=commands[1].sourceline, xpath=tool_xml.getpath(commands[1])) + lint_ctx.error("More than one command tag found, behavior undefined.", **node_props(commands[1], tool_xml)) return if len(commands) == 0: - lint_ctx.error("No command tag found, must specify a command template to execute.", line=root_line, xpath=root_path) + lint_ctx.error("No command tag found, must specify a command template to execute.", **node_props(root, tool_xml)) return command = get_command(tool_xml) if command.text is None: - lint_ctx.error("Command is empty.", line=root_line, xpath=root_path) + lint_ctx.error("Command is empty.", **node_props(root, tool_xml)) elif "TODO" in command.text: - lint_ctx.warn("Command template contains TODO text.", line=command.sourceline, xpath=tool_xml.getpath(command)) + lint_ctx.warn("Command template contains TODO text.", **node_props(command, tool_xml)) command_attrib = command.attrib interpreter_type = None @@ -37,14 +32,14 @@ def lint_command(tool_xml, lint_ctx): 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}]", line=command.sourceline, xpath=tool_xml.getpath(command)) + lint_ctx.warn(f"Unknown detect_errors attribute [{detect_errors}]", **node_props(command, tool_xml)) 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.", line=command.sourceline, xpath=tool_xml.getpath(command)) - lint_ctx.info(f"Tool contains a command{interpreter_info}.", line=command.sourceline, xpath=tool_xml.getpath(command)) + lint_ctx.warn("Command uses deprecated 'interpreter' attribute.", **node_props(command, tool_xml)) + lint_ctx.info(f"Tool contains a command{interpreter_info}.", **node_props(command, tool_xml)) def get_command(tool_xml): diff --git a/lib/galaxy/tool_util/linters/cwl.py b/lib/galaxy/tool_util/linters/cwl.py index 69ddd9efd8b..784961eef20 100644 --- a/lib/galaxy/tool_util/linters/cwl.py +++ b/lib/galaxy/tool_util/linters/cwl.py @@ -14,7 +14,7 @@ def lint_cwl_validation(tool_source, lint_ctx): except Exception as e: validation_exception = e if validation_exception: - lint_ctx.error("Failed to valdiate CWL artifact [%s]", validation_exception) + lint_ctx.error(f"Failed to valdiate CWL artifact: {validation_exception}") else: lint_ctx.info("CWL appears to be valid.") @@ -28,7 +28,7 @@ def lint_new_draft(tool_source, lint_ctx): if cwl_version not in ["v1.0"]: lint_ctx.warn(f"CWL version [{cwl_version}] is unknown, we recommend the v1.0 the stable release.") else: - lint_ctx.info("Modern CWL version [%s]", cwl_version) + lint_ctx.info(f"Modern CWL version [{cwl_version}].") def lint_docker_image(tool_source, lint_ctx): diff --git a/lib/galaxy/tool_util/linters/help.py b/lib/galaxy/tool_util/linters/help.py index 2667966856d..b62392d8bc3 100644 --- a/lib/galaxy/tool_util/linters/help.py +++ b/lib/galaxy/tool_util/linters/help.py @@ -3,42 +3,37 @@ from galaxy.util import ( rst_to_html, unicodify, ) +from ._util import node_props def lint_help(tool_xml, lint_ctx): """Ensure tool contains exactly one valid RST help block.""" - # determine line to report for general problems with help + # determine node to report for general problems with help root = tool_xml.getroot() - if root is not None: - root_line = root.sourceline - root_path = tool_xml.getpath(root) - else: - root_line = 1 - root_path = None helps = root.findall("help") if len(helps) > 1: - lint_ctx.error("More than one help section found, behavior undefined.", line=helps[1].sourceline, xpath=tool_xml.getpath(helps[1])) + lint_ctx.error("More than one help section found, behavior undefined.", **node_props(helps[1], tool_xml)) return if len(helps) == 0: - lint_ctx.warn("No help section found, consider adding a help section to your tool.", line=root_line, xpath=root_path) + lint_ctx.warn("No help section found, consider adding a help section to your tool.", **node_props(root, tool_xml)) return help = helps[0].text or '' if not help.strip(): - lint_ctx.warn("Help section appears to be empty.", line=helps[0].sourceline, xpath=tool_xml.getpath(helps[0])) + lint_ctx.warn("Help section appears to be empty.", **node_props(helps[0], tool_xml)) return - lint_ctx.valid("Tool contains help section.", line=helps[0].sourceline, xpath=tool_xml.getpath(helps[0])) + lint_ctx.valid("Tool contains help section.", **node_props(helps[0], tool_xml)) invalid_rst = rst_invalid(help) if "TODO" in help: - lint_ctx.warn("Help contains TODO text.", line=helps[0].sourceline, xpath=tool_xml.getpath(helps[0])) + lint_ctx.warn("Help contains TODO text.", **node_props(helps[0], tool_xml)) if invalid_rst: - lint_ctx.warn(f"Invalid reStructuredText found in help - [{invalid_rst}].", line=helps[0].sourceline, xpath=tool_xml.getpath(helps[0])) + lint_ctx.warn(f"Invalid reStructuredText found in help - [{invalid_rst}].", **node_props(helps[0], tool_xml)) else: - lint_ctx.valid("Help contains valid reStructuredText.", line=helps[0].sourceline, xpath=tool_xml.getpath(helps[0])) + lint_ctx.valid("Help contains valid reStructuredText.", **node_props(helps[0], tool_xml)) def rst_invalid(text): diff --git a/lib/galaxy/tool_util/linters/inputs.py b/lib/galaxy/tool_util/linters/inputs.py index 93557997224..a800dd58a0d 100644 --- a/lib/galaxy/tool_util/linters/inputs.py +++ b/lib/galaxy/tool_util/linters/inputs.py @@ -1,6 +1,12 @@ """This module contains a linting functions for tool inputs.""" +import re + from galaxy.util import string_as_bool -from ._util import is_datasource, is_valid_cheetah_placeholder +from ._util import ( + is_datasource, + is_valid_cheetah_placeholder, + node_props +) from ..parser.util import _parse_name FILTER_TYPES = [ @@ -46,13 +52,8 @@ PARAMETER_VALIDATOR_TYPE_COMPATIBILITY = { def lint_inputs(tool_xml, lint_ctx): """Lint parameters in a tool's inputs block.""" - # determine line to report for general problems with outputs - try: - tool_line = tool_xml.find("./tool").sourceline - tool_path = tool_xml.getpath(tool_xml.find("./tool")) - except AttributeError: - tool_line = 1 - tool_path = None + # determine node to report for general problems with outputs + tool_node = tool_xml.find("./tool") datasource = is_datasource(tool_xml) inputs = tool_xml.findall("./inputs//param") num_inputs = 0 @@ -60,33 +61,32 @@ def lint_inputs(tool_xml, lint_ctx): 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.", line=param.sourceline, xpath=tool_xml.getpath(param)) + lint_ctx.error("Found param input with no name specified.", **node_props(param, tool_xml)) 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.") + lint_ctx.warn(f"Param input [{param_name}] 'name' attribute is redundant if argument implies the same name.", **node_props(param, tool_xml)) if param_name.strip() == "": - lint_ctx.error("Param input with empty name.", line=param.sourceline, xpath=tool_xml.getpath(param)) + lint_ctx.error("Param input with empty name.", **node_props(param, tool_xml)) elif not is_valid_cheetah_placeholder(param_name): - lint_ctx.warn(f"Param input [{param_name}] is not a valid Cheetah placeholder.", line=param.sourceline, xpath=tool_xml.getpath(param)) + lint_ctx.warn(f"Param input [{param_name}] is not a valid Cheetah placeholder.", **node_props(param, tool_xml)) # TODO lint for params with duplicated name (in inputs & outputs) if "type" not in param_attrib: - lint_ctx.error(f"Param input [{param_name}] input with no type specified.", line=param.sourceline, xpath=tool_xml.getpath(param)) + lint_ctx.error(f"Param input [{param_name}] input with no type specified.", **node_props(param, tool_xml)) continue elif param_attrib["type"].strip() == "": - lint_ctx.error(f"Param input [{param_name}] with empty type specified.", line=param.sourceline, xpath=tool_xml.getpath(param)) + lint_ctx.error(f"Param input [{param_name}] with empty type specified.", **node_props(param, tool_xml)) continue param_type = param_attrib["type"] # TODO lint for valid param type - attribute combinations - # TODO lint required attributes for valid each param type if param_type == "data": if "format" not in param_attrib: - lint_ctx.warn(f"Param input [{param_name}] with no format specified - 'data' format will be assumed.", line=param.sourceline, xpath=tool_xml.getpath(param)) + lint_ctx.warn(f"Param input [{param_name}] with no format specified - 'data' format will be assumed.", **node_props(param, tool_xml)) elif param_type == "select": # get dynamic/statically defined options dynamic_options = param.get("dynamic_options", None) @@ -95,15 +95,15 @@ def lint_inputs(tool_xml, lint_ctx): select_options = param.findall('./option') if dynamic_options is not None: - lint_ctx.warn(f"Select parameter [{param_name}] uses deprecated 'dynamic_options' attribute.", line=param.sourceline, xpath=tool_xml.getpath(param)) + lint_ctx.warn(f"Select parameter [{param_name}] uses deprecated 'dynamic_options' attribute.", **node_props(param, tool_xml)) # check if options are defined by exactly one possibility 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.", line=param.sourceline, xpath=tool_xml.getpath(param)) + 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_props(param, tool_xml)) 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.", line=param.sourceline, xpath=tool_xml.getpath(param)) + lint_ctx.error(f"Select parameter of a conditional [{param_name}] options have to be defined by 'option' children elements.", **node_props(param, tool_xml)) # lint dynamic options if len(options) == 1: @@ -114,10 +114,10 @@ def lint_inputs(tool_xml, lint_ctx): 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.", line=f.sourceline, xpath=tool_xml.getpath(f)) + lint_ctx.error(f"Select parameter [{param_name}] contains filter without type.", **node_props(f, tool_xml)) continue if ftype not in FILTER_TYPES: - lint_ctx.error(f"Select parameter [{param_name}] contains filter with unknown type '{ftype}'.", line=f.sourceline, xpath=tool_xml.getpath(f)) + lint_ctx.error(f"Select parameter [{param_name}] contains filter with unknown type '{ftype}'.", **node_props(f, tool_xml)) continue if ftype in ['add_value', 'data_meta']: filter_adds_options = True @@ -132,33 +132,26 @@ def lint_inputs(tool_xml, lint_ctx): 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.", line=options[0].sourceline, xpath=tool_xml.getpath(options[0])) + 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_props(options[0], tool_xml)) - if from_file is not None: - lint_ctx.warn(f"Select parameter [{param_name}] options uses deprecated 'from_file' attribute.", line=options[0].sourceline, xpath=tool_xml.getpath(options[0])) - if from_parameter is not None: - lint_ctx.warn(f"Select parameter [{param_name}] options uses deprecated 'from_parameter' attribute.", line=options[0].sourceline, xpath=tool_xml.getpath(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_props(options[0], tool_xml)) 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.", line=options[0].sourceline, xpath=tool_xml.getpath(options[0])) + lint_ctx.error(f"Select parameter [{param_name}] options uses 'from_dataset' and 'from_data_table' attribute.", **node_props(options[0], tool_xml)) 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'.", line=options[0].sourceline, xpath=tool_xml.getpath(options[0])) - - if options[0].get("options_filter_attribute", None) is not None: - lint_ctx.warn(f"Select parameter [{param_name}] options uses deprecated 'options_filter_attribute' attribute.", line=options[0].sourceline, xpath=tool_xml.getpath(options[0])) - - if options[0].get("transform_lines", None) is not None: - lint_ctx.warn(f"Select parameter [{param_name}] options uses deprecated 'transform_lines' attribute.", line=options[0].sourceline, xpath=tool_xml.getpath(options[0])) + lint_ctx.error(f"Select parameter [{param_name}] 'meta_file_key' is only compatible with 'from_dataset'.", **node_props(options[0], tool_xml)) elif len(options) > 1: - lint_ctx.error(f"Select parameter [{param_name}] contains multiple options elements.", line=options[1].sourceline, xpath=tool_xml.getpath(options[1])) + lint_ctx.error(f"Select parameter [{param_name}] contains multiple options elements.", **node_props(options[1], tool_xml)) # lint statically defined options if any('value' not in option.attrib for option in select_options): - lint_ctx.error(f"Select parameter [{param_name}] has option without value", line=param.sourceline, xpath=tool_xml.getpath(param)) + lint_ctx.error(f"Select parameter [{param_name}] has option without value", **node_props(param, tool_xml)) if any(option.text is None for option in select_options): - lint_ctx.warn(f"Select parameter [{param_name}] has option without text", line=param.sourceline, xpath=tool_xml.getpath(param)) + lint_ctx.warn(f"Select parameter [{param_name}] has option without text", **node_props(param, tool_xml)) select_options_texts = list() select_options_values = list() @@ -171,22 +164,22 @@ def lint_inputs(tool_xml, lint_ctx): 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", line=param.sourceline, xpath=tool_xml.getpath(param)) + lint_ctx.error(f"Select parameter [{param_name}] has multiple options with the same text content", **node_props(param, tool_xml)) 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", line=param.sourceline, xpath=tool_xml.getpath(param)) + lint_ctx.error(f"Select parameter [{param_name}] has multiple options with the same value", **node_props(param, tool_xml)) 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 multiple: - lint_ctx.error(f'Select [{param_name}] `display="checkboxes"` is incompatible with `multiple="false"`, remove the `display` attribute', line=param.sourceline, xpath=tool_xml.getpath(param)) + lint_ctx.error(f'Select [{param_name}] `display="checkboxes"` is incompatible with `multiple="false"`, remove the `display` attribute', **node_props(param, tool_xml)) if not optional: - lint_ctx.error(f'Select [{param_name}] `display="checkboxes"` is incompatible with `optional="false"`, remove the `display` attribute', line=param.sourceline, xpath=tool_xml.getpath(param)) + lint_ctx.error(f'Select [{param_name}] `display="checkboxes"` is incompatible with `optional="false"`, remove the `display` attribute', **node_props(param, tool_xml)) if param_attrib.get("display") == "radio": if multiple: - lint_ctx.error(f'Select [{param_name}] display="radio" is incompatible with multiple="true"', line=param.sourceline, xpath=tool_xml.getpath(param)) + lint_ctx.error(f'Select [{param_name}] display="radio" is incompatible with multiple="true"', **node_props(param, tool_xml)) if optional: - lint_ctx.error(f'Select [{param_name}] display="radio" is incompatible with optional="true"', line=param.sourceline, xpath=tool_xml.getpath(param)) + lint_ctx.error(f'Select [{param_name}] display="radio" is incompatible with optional="true"', **node_props(param, tool_xml)) # TODO: Validate type, much more... # lint validators @@ -196,40 +189,46 @@ def lint_inputs(tool_xml, lint_ctx): vtype = validator.attrib['type'] 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}'", line=validator.sourceline, xpath=tool_xml.getpath(validator)) + lint_ctx.error(f"Parameter [{param_name}]: validator with an incompatible type '{vtype}'", **node_props(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}'", line=validator.sourceline, xpath=tool_xml.getpath(validator)) - if vtype == "expression" and validator.text is None: - lint_ctx.error(f"Parameter [{param_name}]: expression validator without content", line=validator.sourceline, xpath=tool_xml.getpath(validator)) + lint_ctx.error(f"Parameter [{param_name}]: attribute '{attrib}' is incompatible with validator of type '{vtype}'", **node_props(validator, tool_xml)) + if vtype == "expression": + if validator.text is None: + lint_ctx.error(f"Parameter [{param_name}]: expression validators are expected to contain text", **node_props(validator, tool_xml)) + else: + try: + re.compile(validator.text) + except Exception as e: + lint_ctx.error(f"Parameter [{param_name}]: '{validator.text}' is no valid regular expression: {str(e)}", **node_props(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}')", line=validator.sourceline, xpath=tool_xml.getpath(validator)) + lint_ctx.warn(f"Parameter [{param_name}]: '{vtype}' validators are not expected to contain text (found '{validator.text}')", **node_props(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): - lint_ctx.error(f"Parameter [{param_name}]: '{vtype}' validators need to define the 'min' or 'max' attribute(s)", line=validator.sourceline, xpath=tool_xml.getpath(validator)) + lint_ctx.error(f"Parameter [{param_name}]: '{vtype}' validators need to define the 'min' or 'max' attribute(s)", **node_props(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)", line=validator.sourceline, xpath=tool_xml.getpath(validator)) + lint_ctx.error(f"Parameter [{param_name}]: '{vtype}' validators need to define the 'check' or 'skip' attribute(s)", **node_props(validator, tool_xml)) if vtype in ["value_in_data_table", "value_not_in_data_table", "dataset_metadata_in_data_table", "dataset_metadata_not_in_data_table"] and "table_name" not in validator.attrib: - lint_ctx.error(f"Parameter [{param_name}]: '{vtype}' validators need to define the 'table_name' attribute", line=validator.sourceline, xpath=tool_xml.getpath(validator)) + lint_ctx.error(f"Parameter [{param_name}]: '{vtype}' validators need to define the 'table_name' attribute", **node_props(validator, tool_xml)) conditional_selects = tool_xml.findall("./inputs//conditional") for conditional in conditional_selects: conditional_name = conditional.get('name') if not conditional_name: - lint_ctx.error("Conditional without a name", line=conditional.sourceline, xpath=tool_xml.getpath(conditional)) + lint_ctx.error("Conditional without a name", **node_props(conditional, tool_xml)) 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)}", line=conditional.sourceline, xpath=tool_xml.getpath(conditional)) + lint_ctx.error(f"Conditional [{conditional_name}] needs exactly one child found {len(first_param)}", **node_props(conditional, tool_xml)) continue first_param = first_param[0] first_param_type = first_param.get('type') - if first_param_type not in ['select', 'boolean']: - lint_ctx.error(f'Conditional [{conditional_name}] first param should have type="select" (or type="boolean" which is discouraged)', line=first_param.sourceline, xpath=tool_xml.getpath(first_param)) + if first_param_type == 'boolean': + lint_ctx.warn(f'Conditional [{conditional_name}] first param of type="boolean" is discouraged, use a select', **node_props(first_param, tool_xml)) + elif first_param_type != 'select': + lint_ctx.error(f'Conditional [{conditional_name}] first param should have type="select"', **node_props(first_param, tool_xml)) continue - elif first_param_type == 'boolean': - lint_ctx.warn(f'Conditional [{conditional_name}] first param of type="boolean" is discouraged, use a select', line=first_param.sourceline, xpath=tool_xml.getpath(first_param)) if first_param_type == 'select': select_options = _find_with_attribute(first_param, 'option', 'value') @@ -242,38 +241,38 @@ def lint_inputs(tool_xml, lint_ctx): 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"', line=first_param.sourceline, xpath=tool_xml.getpath(first_param)) + lint_ctx.warn(f'Conditional [{conditional_name}] test parameter cannot be {incomp}="true"', **node_props(first_param, tool_xml)) 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", line=conditional.sourceline, xpath=tool_xml.getpath(conditional)) + lint_ctx.error(f"Conditional [{conditional_name}] when without value", **node_props(conditional, tool_xml)) when_ids = [w.get('value') for w in whens if w.get('value') is not None] for option_id in option_ids: if option_id not in when_ids: - lint_ctx.warn(f"Conditional [{conditional_name}] no block found for {first_param_type} option '{option_id}'", line=conditional.sourceline, xpath=tool_xml.getpath(conditional)) + lint_ctx.warn(f"Conditional [{conditional_name}] no block found for {first_param_type} option '{option_id}'", **node_props(conditional, tool_xml)) 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