From 3ed87bff0a88c43c5f5018023d8f2b8e2fbd0323 Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Tue, 6 Sep 2022 14:01:14 +0200 Subject: [PATCH 01/13] linter: allow options elements in data params linting of valid childs for params has been added here: https://github.com/galaxyproject/galaxy/pull/12232 options for data params has been forgotten in addition also checks for valid attribs and filter types has been added to linting --- lib/galaxy/tool_util/linters/inputs.py | 19 ++++++++- test/unit/tool_util/test_tool_linters.py | 52 +++++++++++++++++++++++- 2 files changed, 69 insertions(+), 2 deletions(-) diff --git a/lib/galaxy/tool_util/linters/inputs.py b/lib/galaxy/tool_util/linters/inputs.py index 6a7e602f7c0..3fcf9b5068f 100644 --- a/lib/galaxy/tool_util/linters/inputs.py +++ b/lib/galaxy/tool_util/linters/inputs.py @@ -108,7 +108,7 @@ PARAMETER_VALIDATOR_TYPE_COMPATIBILITY = { } PARAM_TYPE_CHILD_COMBINATIONS = [ - ("./options", ["select", "drill_down"]), + ("./options", ["data", "select", "drill_down"]), ("./options/option", ["drill_down"]), ("./column", ["data_column"]), ] @@ -167,6 +167,23 @@ def lint_inputs(tool_xml, lint_ctx): lint_ctx.warn( f"Param input [{param_name}] with no format specified - 'data' format will be assumed.", node=param ) + options = param.findall("./options") + if len(options) == 1: + if len(options[0].attrib) > 0: + lint_ctx.error( + f"Data parameter [{param_name}] uses invalid attributes: {options[0].attrib}", node=param + ) + elif 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 f.get("key") != "dbkey" or f.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, + ) + elif param_type == "select": # get dynamic/statically defined options dynamic_options = param.get("dynamic_options", None) diff --git a/test/unit/tool_util/test_tool_linters.py b/test/unit/tool_util/test_tool_linters.py index cbc0a2fc88d..2d28f3b8531 100644 --- a/test/unit/tool_util/test_tool_linters.py +++ b/test/unit/tool_util/test_tool_linters.py @@ -190,6 +190,31 @@ INPUTS_DATA_PARAM = """ """ +INPUTS_DATA_PARAM_OPTIONS = """ + + + + + + + + + +""" + +INPUTS_DATA_PARAM_INVALIDOPTIONS = """ + + + + + + + + + + +""" + INPUTS_CONDITIONAL = """ @@ -1012,6 +1037,31 @@ def test_inputs_data_param(lint_ctx): assert not lint_ctx.error_messages +def test_inputs_data_param_options(lint_ctx): + tool_source = get_xml_tool_source(INPUTS_DATA_PARAM_OPTIONS) + run_lint(lint_ctx, inputs.lint_inputs, tool_source) + assert not lint_ctx.valid_messages + assert "Found 1 input parameters." in lint_ctx.info_messages + assert len(lint_ctx.info_messages) == 1 + assert not lint_ctx.warn_messages + assert not lint_ctx.error_messages + + +def test_inputs_data_param_invalidoptions(lint_ctx): + tool_source = get_xml_tool_source(INPUTS_DATA_PARAM_INVALIDOPTIONS) + run_lint(lint_ctx, inputs.lint_inputs, tool_source) + assert not lint_ctx.valid_messages + assert "Found 1 input parameters." in lint_ctx.info_messages + assert len(lint_ctx.info_messages) == 1 + assert not lint_ctx.warn_messages + assert "Data parameter [valid_name] contains multiple options elements." in lint_ctx.error_messages + assert ( + 'Data parameter [valid_name] for filters only type="data_meta" and key="dbkey" are allowed, found type="expression" and key="None"' + in lint_ctx.error_messages + ) + assert len(lint_ctx.error_messages) == 2 + + def test_inputs_conditional(lint_ctx): tool_source = get_xml_tool_source(INPUTS_CONDITIONAL) run_lint(lint_ctx, inputs.lint_inputs, tool_source) @@ -1197,7 +1247,7 @@ def test_inputs_type_child_combinations(lint_ctx): assert not lint_ctx.valid_messages assert not lint_ctx.warn_messages assert ( - "Parameter [text_param] './options' tags are only allowed for parameters of type ['select', 'drill_down']" + "Parameter [text_param] './options' tags are only allowed for parameters of type ['data', 'select', 'drill_down']" in lint_ctx.error_messages ) assert ( From 6ebfa1ca6620203d75d319d912dd7fe002ca8ec1 Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Tue, 6 Sep 2022 14:20:56 +0200 Subject: [PATCH 02/13] also improve docs --- lib/galaxy/tool_util/xsd/galaxy.xsd | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/lib/galaxy/tool_util/xsd/galaxy.xsd b/lib/galaxy/tool_util/xsd/galaxy.xsd index 90cc1ccf6dd..603f40d9b0b 100644 --- a/lib/galaxy/tool_util/xsd/galaxy.xsd +++ b/lib/galaxy/tool_util/xsd/galaxy.xsd @@ -3968,14 +3968,18 @@ dataset for the contained input of the type specified using the ``type`` tag. `` tag when the ``type`` attribute value is ``select`` or -``data`` and used to dynamically generated lists of options. This tag set -dynamically creates a list of options whose values can be -obtained from a predefined file stored locally or a dataset selected from the -current history. +``data`` and used to dynamically generated lists of options. + +For data parameters it can only be used to restrict inputs to datasets having +the same ``dbkey`` like another input by using a ``data_meta`` filter. See for +instance here: [/tools/maf/interval2maf.xml](https://github.com/galaxyproject/galaxy/blob/master/tools/maf/interval2maf.xml) + +For select parameters this tag set dynamically creates a list of options whose +values can be obtained from a predefined file stored locally or a dataset +selected from the current history. There are at least five basic ways to use this tag - four of these correspond to a ``from_XXX`` attribute on the ``options`` directive and the other is to From 1cb1ac3dfd4649a45071c10882fd7f2fcb6e8597 Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Mon, 19 Sep 2022 10:24:49 +0200 Subject: [PATCH 03/13] adapt linter to check also for options_filter_attribute --- lib/galaxy/tool_util/linters/inputs.py | 26 +++++++++++++++++------ test/unit/tool_util/test_tool_linters.py | 27 ++++++++++++++++++++++-- 2 files changed, 45 insertions(+), 8 deletions(-) diff --git a/lib/galaxy/tool_util/linters/inputs.py b/lib/galaxy/tool_util/linters/inputs.py index 3fcf9b5068f..16455b51a32 100644 --- a/lib/galaxy/tool_util/linters/inputs.py +++ b/lib/galaxy/tool_util/linters/inputs.py @@ -168,21 +168,35 @@ def lint_inputs(tool_xml, lint_ctx): f"Param input [{param_name}] with no format specified - 'data' format will be assumed.", node=param ) options = param.findall("./options") + has_options_filter_attribute = False if len(options) == 1: - if len(options[0].attrib) > 0: - lint_ctx.error( - f"Data parameter [{param_name}] uses invalid attributes: {options[0].attrib}", node=param - ) + 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: 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 f.get("key") != "dbkey" or f.get("type") != "data_meta": + if not f.get("ref"): 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")}"', + 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": + lint_ctx.error( + f'Data parameter [{param_name}] for filters only type="data_meta" is allowed, found type="{f.get("type")}"', + node=f, + ) + else: + if f.get("key") != "dbkey" or f.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, + ) elif param_type == "select": # get dynamic/statically defined options diff --git a/test/unit/tool_util/test_tool_linters.py b/test/unit/tool_util/test_tool_linters.py index 2d28f3b8531..533dcd31e36 100644 --- a/test/unit/tool_util/test_tool_linters.py +++ b/test/unit/tool_util/test_tool_linters.py @@ -195,7 +195,19 @@ INPUTS_DATA_PARAM_OPTIONS = """ - + + + + + +""" + +INPUTS_DATA_PARAM_OPTIONS_FILTER_ATTRIBUTE = """ + + + + + @@ -1047,6 +1059,16 @@ def test_inputs_data_param_options(lint_ctx): assert not lint_ctx.error_messages +def test_inputs_data_param_options_filter_attribute(lint_ctx): + tool_source = get_xml_tool_source(INPUTS_DATA_PARAM_OPTIONS_FILTER_ATTRIBUTE) + run_lint(lint_ctx, inputs.lint_inputs, tool_source) + assert not lint_ctx.valid_messages + assert "Found 1 input parameters." in lint_ctx.info_messages + assert len(lint_ctx.info_messages) == 1 + assert not lint_ctx.warn_messages + assert not lint_ctx.error_messages + + def test_inputs_data_param_invalidoptions(lint_ctx): tool_source = get_xml_tool_source(INPUTS_DATA_PARAM_INVALIDOPTIONS) run_lint(lint_ctx, inputs.lint_inputs, tool_source) @@ -1055,11 +1077,12 @@ def test_inputs_data_param_invalidoptions(lint_ctx): assert len(lint_ctx.info_messages) == 1 assert not lint_ctx.warn_messages assert "Data parameter [valid_name] contains multiple options elements." in lint_ctx.error_messages + assert "Data parameter [valid_name] filter needs to define a ref attribute" in lint_ctx.error_messages assert ( 'Data parameter [valid_name] for filters only type="data_meta" and key="dbkey" are allowed, found type="expression" and key="None"' in lint_ctx.error_messages ) - assert len(lint_ctx.error_messages) == 2 + assert len(lint_ctx.error_messages) == 3 def test_inputs_conditional(lint_ctx): From e7d738569c3215ece224d567e703f9dcc6b6121d Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Sun, 29 Jan 2023 13:50:13 +0100 Subject: [PATCH 04/13] add unit test for is_a_yaml_with_class as used in planemo --- test/unit/tool_util/test_loader_directory.py | 23 ++++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 test/unit/tool_util/test_loader_directory.py diff --git a/test/unit/tool_util/test_loader_directory.py b/test/unit/tool_util/test_loader_directory.py new file mode 100644 index 00000000000..8a35da215d8 --- /dev/null +++ b/test/unit/tool_util/test_loader_directory.py @@ -0,0 +1,23 @@ +import os +import tempfile + +from galaxy.tool_util.loader_directory import is_a_yaml_with_class + +def test_is_a_yaml_with_class(): + with tempfile.NamedTemporaryFile("w", suffix=".yaml", delete=False) as tf: + fname = tf.name + tf.write("""class: GalaxyWorkflow +name: "Test Workflow" +inputs: + - id: input1 +outputs: + - id: wf_output_1 + outputSource: first_cat/out_file1 +steps: + - tool_id: cat + label: first_cat + in: + input1: input1""") + + assert is_a_yaml_with_class(fname, ["GalaxyWorkflow"]) + os.unlink(fname) \ No newline at end of file From 4c4cc2c75bc998b1f52f6b3d6e0e3c2c34337cec Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Sun, 29 Jan 2023 13:14:30 +0100 Subject: [PATCH 05/13] fix looks_like_yaml_or_cwl_with_class the string `class: ...` might appear on the first line hence the leading `\n` was wrong I guess its better to use `^` and `$` and add the MULTILINE flag --- lib/galaxy/tool_util/loader_directory.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/galaxy/tool_util/loader_directory.py b/lib/galaxy/tool_util/loader_directory.py index fd0e73e58f5..1491f5ab047 100644 --- a/lib/galaxy/tool_util/loader_directory.py +++ b/lib/galaxy/tool_util/loader_directory.py @@ -190,7 +190,7 @@ def looks_like_a_data_manager_xml(path): def as_dict_if_looks_like_yaml_or_cwl_with_class(path, classes): """ - get a dict from yaml file if it contains `class: CLASS`, where CLASS is + get a dict from yaml file if it contains a line `class: CLASS`, where CLASS is any string given in CLASSES. must appear in the first 5k and also load properly in total. """ @@ -199,7 +199,7 @@ def as_dict_if_looks_like_yaml_or_cwl_with_class(path, classes): start_contents = f.read(5 * 1024) except UnicodeDecodeError: return False, None - if re.search(rf"\nclass:\s+({'|'.join(classes)})\s*\n", start_contents) is None: + if re.search(rf"^class:\s+{'|'.join(classes)}\s*$", start_contents, re.MULTILINE) is None: return False, None with open(path) as f: From 63b0c7e23b2614341646cef07be6c602017547d7 Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Sun, 29 Jan 2023 15:45:21 +0100 Subject: [PATCH 06/13] linter fixes --- test/unit/tool_util/test_loader_directory.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/unit/tool_util/test_loader_directory.py b/test/unit/tool_util/test_loader_directory.py index 8a35da215d8..3b808caa1c6 100644 --- a/test/unit/tool_util/test_loader_directory.py +++ b/test/unit/tool_util/test_loader_directory.py @@ -3,6 +3,7 @@ import tempfile from galaxy.tool_util.loader_directory import is_a_yaml_with_class + def test_is_a_yaml_with_class(): with tempfile.NamedTemporaryFile("w", suffix=".yaml", delete=False) as tf: fname = tf.name @@ -20,4 +21,4 @@ steps: input1: input1""") assert is_a_yaml_with_class(fname, ["GalaxyWorkflow"]) - os.unlink(fname) \ No newline at end of file + os.unlink(fname) From 92d06c26e7fc13db96e798ae3e64a20923005a20 Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Sun, 29 Jan 2023 16:05:32 +0100 Subject: [PATCH 07/13] black formatting fixes --- test/unit/tool_util/test_loader_directory.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test/unit/tool_util/test_loader_directory.py b/test/unit/tool_util/test_loader_directory.py index 3b808caa1c6..6b25364349a 100644 --- a/test/unit/tool_util/test_loader_directory.py +++ b/test/unit/tool_util/test_loader_directory.py @@ -7,7 +7,8 @@ from galaxy.tool_util.loader_directory import is_a_yaml_with_class def test_is_a_yaml_with_class(): with tempfile.NamedTemporaryFile("w", suffix=".yaml", delete=False) as tf: fname = tf.name - tf.write("""class: GalaxyWorkflow + tf.write( + """class: GalaxyWorkflow name: "Test Workflow" inputs: - id: input1 @@ -18,7 +19,8 @@ steps: - tool_id: cat label: first_cat in: - input1: input1""") + input1: input1""" + ) assert is_a_yaml_with_class(fname, ["GalaxyWorkflow"]) os.unlink(fname) From 8ec08f6665894fb5ab2de8387b44daa1745e8be9 Mon Sep 17 00:00:00 2001 From: M Bernt Date: Mon, 30 Jan 2023 10:38:29 +0100 Subject: [PATCH 08/13] Test improvements Co-authored-by: Marius van den Beek --- test/unit/tool_util/test_loader_directory.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/test/unit/tool_util/test_loader_directory.py b/test/unit/tool_util/test_loader_directory.py index 6b25364349a..08ec1f5a51c 100644 --- a/test/unit/tool_util/test_loader_directory.py +++ b/test/unit/tool_util/test_loader_directory.py @@ -1,11 +1,10 @@ -import os import tempfile from galaxy.tool_util.loader_directory import is_a_yaml_with_class def test_is_a_yaml_with_class(): - with tempfile.NamedTemporaryFile("w", suffix=".yaml", delete=False) as tf: + with tempfile.NamedTemporaryFile("w", suffix=".yaml") as tf: fname = tf.name tf.write( """class: GalaxyWorkflow @@ -21,6 +20,5 @@ steps: in: input1: input1""" ) - - assert is_a_yaml_with_class(fname, ["GalaxyWorkflow"]) - os.unlink(fname) + tf.flush() + assert is_a_yaml_with_class(fname, ["GalaxyWorkflow"]) From fb60ec257f7e0d62364c8c0d640cb2a5ad290a39 Mon Sep 17 00:00:00 2001 From: M Bernt Date: Mon, 30 Jan 2023 11:26:50 +0100 Subject: [PATCH 09/13] Fix test name typo Co-authored-by: Marius van den Beek --- test/unit/tool_util/test_tool_linters.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/tool_util/test_tool_linters.py b/test/unit/tool_util/test_tool_linters.py index 533dcd31e36..ff722d2e091 100644 --- a/test/unit/tool_util/test_tool_linters.py +++ b/test/unit/tool_util/test_tool_linters.py @@ -1069,7 +1069,7 @@ def test_inputs_data_param_options_filter_attribute(lint_ctx): assert not lint_ctx.error_messages -def test_inputs_data_param_invalidoptions(lint_ctx): +def test_inputs_data_param_invalid_options(lint_ctx): tool_source = get_xml_tool_source(INPUTS_DATA_PARAM_INVALIDOPTIONS) run_lint(lint_ctx, inputs.lint_inputs, tool_source) assert not lint_ctx.valid_messages From 327e4221895ff67efe4accb5eea98606f594162f Mon Sep 17 00:00:00 2001 From: M Bernt Date: Mon, 30 Jan 2023 12:40:05 +0100 Subject: [PATCH 10/13] Reformulate doc --- lib/galaxy/tool_util/xsd/galaxy.xsd | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/galaxy/tool_util/xsd/galaxy.xsd b/lib/galaxy/tool_util/xsd/galaxy.xsd index 603f40d9b0b..7db7caf5928 100644 --- a/lib/galaxy/tool_util/xsd/galaxy.xsd +++ b/lib/galaxy/tool_util/xsd/galaxy.xsd @@ -3973,8 +3973,7 @@ for an example of how to use this tag set. This tag set is optionally contained within the ```` tag when the ``type`` attribute value is ``select`` or ``data`` and used to dynamically generated lists of options. -For data parameters it can only be used to restrict inputs to datasets having -the same ``dbkey`` like another input by using a ``data_meta`` filter. See for +For data parameters this tag can be used to restrict possible input datasets to datasets that match the ``dbkey`` of another data input by including a ``data_meta`` filter. See for instance here: [/tools/maf/interval2maf.xml](https://github.com/galaxyproject/galaxy/blob/master/tools/maf/interval2maf.xml) For select parameters this tag set dynamically creates a list of options whose From c0eb66173ea9d2fd0e24d69bd23705a67b2582df Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Tue, 31 Jan 2023 15:33:14 +0100 Subject: [PATCH 11/13] restore rst_invalid function used in planemo https://github.com/galaxyproject/planemo/pull/1275 has been removed here https://github.com/galaxyproject/galaxy/pull/14588 --- lib/galaxy/tool_util/linters/help.py | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/lib/galaxy/tool_util/linters/help.py b/lib/galaxy/tool_util/linters/help.py index 1d5776a8c56..c47eb848cb7 100644 --- a/lib/galaxy/tool_util/linters/help.py +++ b/lib/galaxy/tool_util/linters/help.py @@ -30,10 +30,22 @@ def lint_help(tool_xml, lint_ctx): if "TODO" in help_text: lint_ctx.warn("Help contains TODO text.", node=helps[0]) - try: - rst_to_html(help_text, error=True) - except Exception as e: - lint_ctx.warn(f"Invalid reStructuredText found in help - [{unicodify(e)}].", node=helps[0]) - return + 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]) - lint_ctx.valid("Help contains valid reStructuredText.", node=helps[0]) + +def rst_invalid(text): + """ + Predicate to determine if text is invalid reStructuredText. + Return False if the supplied text is valid reStructuredText or + a string indicating the problem. + """ + invalid_rst = False + try: + rst_to_html(text, error=True) + except Exception as e: + invalid_rst = unicodify(e) + return invalid_rst From 8b8e0431e925bf18a4c94baf4f5f6a4abb167ffc Mon Sep 17 00:00:00 2001 From: M Bernt Date: Tue, 31 Jan 2023 15:49:45 +0100 Subject: [PATCH 12/13] Add type annotation Co-authored-by: Marius van den Beek --- lib/galaxy/tool_util/linters/help.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/galaxy/tool_util/linters/help.py b/lib/galaxy/tool_util/linters/help.py index c47eb848cb7..62c25f168f2 100644 --- a/lib/galaxy/tool_util/linters/help.py +++ b/lib/galaxy/tool_util/linters/help.py @@ -43,7 +43,7 @@ def rst_invalid(text): Return False if the supplied text is valid reStructuredText or a string indicating the problem. """ - invalid_rst = False + invalid_rst: Union[bool, str] = False try: rst_to_html(text, error=True) except Exception as e: From e0ab580e5f6b5abd0876293391e5f82641daf878 Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Tue, 31 Jan 2023 16:00:25 +0100 Subject: [PATCH 13/13] add Union import and type annotate function --- lib/galaxy/tool_util/linters/help.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/galaxy/tool_util/linters/help.py b/lib/galaxy/tool_util/linters/help.py index 62c25f168f2..0008d465662 100644 --- a/lib/galaxy/tool_util/linters/help.py +++ b/lib/galaxy/tool_util/linters/help.py @@ -1,4 +1,7 @@ """This module contains a linting function for a tool's help.""" + +from typing import Union + from galaxy.util import ( rst_to_html, unicodify, @@ -37,7 +40,7 @@ def lint_help(tool_xml, lint_ctx): lint_ctx.valid("Help contains valid reStructuredText.", node=helps[0]) -def rst_invalid(text): +def rst_invalid(text: str) -> Union[bool, str]: """ Predicate to determine if text is invalid reStructuredText. Return False if the supplied text is valid reStructuredText or