From 5851bd8966f4cd04c9437e8d08c8f7248c3ef2bc Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Fri, 3 May 2024 12:48:07 +0200 Subject: [PATCH] linting: restore possibiity to skip old linter function names until 23.2 we had linter functions lint_xyz in linter modules xyz. with 24.0 we splited the functions linter classes. lint contexts allow to skip linters by name (xyz for the old functions now the class name), i.e. we can not skip by the old names anymore. this may break planemo command lines xref https://github.com/galaxyproject/planemo/pull/1441#discussion_r1587995463. here I suggest to add a check for the module name against the names listed in the skip list. this restores the possibility to skip by old linter names and adds the possibility to skip whole linter modules (which might be handy anyway). tried to add some tests that hopefully help to maintain this functionality. --- lib/galaxy/tool_util/lint.py | 13 ++- .../linters/{outputs.py => output.py} | 0 test/unit/tool_util/test_tool_linters.py | 91 ++++++++++++++++--- 3 files changed, 91 insertions(+), 13 deletions(-) rename lib/galaxy/tool_util/linters/{outputs.py => output.py} (100%) diff --git a/lib/galaxy/tool_util/lint.py b/lib/galaxy/tool_util/lint.py index bfde64c9d1bd..599006457349 100644 --- a/lib/galaxy/tool_util/lint.py +++ b/lib/galaxy/tool_util/lint.py @@ -212,11 +212,19 @@ def found_errors(self) -> bool: def found_warns(self) -> bool: return len(self.warn_messages) > 0 - def lint(self, name: str, lint_func: Callable[[LintTargetType, "LintContext"], None], lint_target: LintTargetType): + def lint( + self, + name: str, + lint_func: Callable[[LintTargetType, "LintContext"], None], + lint_target: LintTargetType, + module_name: Optional[str] = None, + ): if name.startswith("lint_"): name = name[len("lint_") :] if name in self.skip_types: return + if module_name and module_name in self.skip_types: + return if self.level < LintLevel.SILENT: # this is a relict from the past where the lint context @@ -355,6 +363,7 @@ def lint_tool_source_with_modules(lint_context: LintContext, tool_source, linter tool_type = tool_source.parse_tool_type() or "default" for module in linter_modules: + module_name = module.__name__.split(".")[-1] lint_tool_types = getattr(module, "lint_tool_types", ["default", "manage_data"]) if not ("*" in lint_tool_types or tool_type in lint_tool_types): continue @@ -374,7 +383,7 @@ def lint_tool_source_with_modules(lint_context: LintContext, tool_source, linter else: lint_context.lint(name, value, tool_source) elif inspect.isclass(value) and issubclass(value, Linter) and not inspect.isabstract(value): - lint_context.lint(name, value.lint, tool_source) + lint_context.lint(name, value.lint, tool_source, module_name=module_name) return lint_context diff --git a/lib/galaxy/tool_util/linters/outputs.py b/lib/galaxy/tool_util/linters/output.py similarity index 100% rename from lib/galaxy/tool_util/linters/outputs.py rename to lib/galaxy/tool_util/linters/output.py diff --git a/test/unit/tool_util/test_tool_linters.py b/test/unit/tool_util/test_tool_linters.py index e553d2c16884..7b106c497d1c 100644 --- a/test/unit/tool_util/test_tool_linters.py +++ b/test/unit/tool_util/test_tool_linters.py @@ -1,8 +1,10 @@ +import inspect import os import tempfile import pytest +import galaxy.tool_util.linters from galaxy.tool_util.lint import ( lint_tool_source_with, lint_tool_source_with_modules, @@ -18,7 +20,7 @@ general, help, inputs, - outputs, + output, stdio, tests, xml_order, @@ -29,6 +31,7 @@ from galaxy.util import ( ElementTree, parse_xml, + submodules, ) from galaxy.util.xml_macros import load_with_references @@ -1508,7 +1511,7 @@ def test_inputs_repeats(lint_ctx): def test_outputs_missing(lint_ctx): tool_source = get_xml_tool_source(OUTPUTS_MISSING) - run_lint_module(lint_ctx, outputs, tool_source) + run_lint_module(lint_ctx, output, tool_source) assert "Tool contains no outputs section, most tools should produce outputs." in lint_ctx.warn_messages assert not lint_ctx.info_messages assert not lint_ctx.valid_messages @@ -1518,7 +1521,7 @@ def test_outputs_missing(lint_ctx): def test_outputs_multiple(lint_ctx): tool_source = get_xml_tool_source(OUTPUTS_MULTIPLE) - run_lint_module(lint_ctx, outputs, tool_source) + run_lint_module(lint_ctx, output, tool_source) assert "0 outputs found." in lint_ctx.info_messages assert "Invalid XML: Element 'outputs': This element is not expected." in lint_ctx.error_messages assert len(lint_ctx.info_messages) == 1 @@ -1535,7 +1538,7 @@ def test_outputs_unknown_tag(lint_ctx): probably be avoided. """ tool_source = get_xml_tool_source(OUTPUTS_UNKNOWN_TAG) - lint_tool_source_with_modules(lint_ctx, tool_source, [outputs, xsd]) + lint_tool_source_with_modules(lint_ctx, tool_source, [output, xsd]) assert "0 outputs found." in lint_ctx.info_messages assert "Avoid the use of 'output' and replace by 'data' or 'collection'" in lint_ctx.warn_messages assert len(lint_ctx.info_messages) == 1 @@ -1546,7 +1549,7 @@ def test_outputs_unknown_tag(lint_ctx): def test_outputs_unnamed_invalid_name(lint_ctx): tool_source = get_xml_tool_source(OUTPUTS_UNNAMED_INVALID_NAME) - run_lint_module(lint_ctx, outputs, tool_source) + run_lint_module(lint_ctx, output, tool_source) assert "3 outputs found." in lint_ctx.info_messages assert "Invalid XML: Element 'data': The attribute 'name' is required but missing." in lint_ctx.error_messages assert "Invalid XML: Element 'collection': The attribute 'name' is required but missing." in lint_ctx.error_messages @@ -1562,7 +1565,7 @@ def test_outputs_unnamed_invalid_name(lint_ctx): def test_outputs_format_input_legacy(lint_ctx): tool_source = get_xml_tool_source(OUTPUTS_FORMAT_INPUT_LEGACY) - run_lint_module(lint_ctx, outputs, tool_source) + run_lint_module(lint_ctx, output, tool_source) assert "1 outputs found." in lint_ctx.info_messages assert "Using format='input' on data is deprecated. Use the format_source attribute." in lint_ctx.warn_messages assert len(lint_ctx.info_messages) == 1 @@ -1573,7 +1576,7 @@ def test_outputs_format_input_legacy(lint_ctx): def test_outputs_format_input(lint_ctx): tool_source = get_xml_tool_source(OUTPUTS_FORMAT_INPUT) - run_lint_module(lint_ctx, outputs, tool_source) + run_lint_module(lint_ctx, output, tool_source) assert "1 outputs found." in lint_ctx.info_messages assert "Using format='input' on data is deprecated. Use the format_source attribute." in lint_ctx.error_messages assert len(lint_ctx.info_messages) == 1 @@ -1584,7 +1587,7 @@ def test_outputs_format_input(lint_ctx): def test_outputs_collection_format_source(lint_ctx): tool_source = get_xml_tool_source(OUTPUTS_COLLECTION_FORMAT_SOURCE) - run_lint_module(lint_ctx, outputs, tool_source) + run_lint_module(lint_ctx, output, tool_source) assert "Tool data output 'reverse' should use either format_source or format/ext" in lint_ctx.warn_messages assert len(lint_ctx.info_messages) == 1 assert not lint_ctx.valid_messages @@ -1594,7 +1597,7 @@ def test_outputs_collection_format_source(lint_ctx): def test_outputs_format_action(lint_ctx): tool_source = get_xml_tool_source(OUTPUTS_FORMAT_ACTION) - run_lint_module(lint_ctx, outputs, tool_source) + run_lint_module(lint_ctx, output, tool_source) assert len(lint_ctx.info_messages) == 1 assert not lint_ctx.valid_messages assert not lint_ctx.warn_messages @@ -1603,7 +1606,7 @@ def test_outputs_format_action(lint_ctx): def test_outputs_discover_tool_provided_metadata(lint_ctx): tool_source = get_xml_tool_source(OUTPUTS_DISCOVER_TOOL_PROVIDED_METADATA) - run_lint_module(lint_ctx, outputs, tool_source) + run_lint_module(lint_ctx, output, tool_source) assert "1 outputs found." in lint_ctx.info_messages assert len(lint_ctx.info_messages) == 1 assert not lint_ctx.valid_messages @@ -1614,7 +1617,7 @@ def test_outputs_discover_tool_provided_metadata(lint_ctx): def test_outputs_duplicated_name_label(lint_ctx): """ """ tool_source = get_xml_tool_source(OUTPUTS_DUPLICATED_NAME_LABEL) - run_lint_module(lint_ctx, outputs, tool_source) + run_lint_module(lint_ctx, output, tool_source) assert "4 outputs found." in lint_ctx.info_messages assert len(lint_ctx.info_messages) == 1 assert not lint_ctx.valid_messages @@ -2075,6 +2078,30 @@ def test_xml_comments_are_ignored(lint_ctx: LintContext): assert "Comment" not in lint_message.message +def test_skip_by_name(lint_ctx): + # add a linter class name to the skip list + lint_ctx.skip_types = ["CitationsMissing"] + + tool_source = get_xml_tool_source(CITATIONS_ABSENT) + run_lint_module(lint_ctx, citations, tool_source) + assert not lint_ctx.warn_messages + assert not lint_ctx.info_messages + assert not lint_ctx.valid_messages + assert not lint_ctx.error_messages + + +def test_skip_by_name(lint_ctx): + # add a module name to the skip list -> all linters in this module are skipped + lint_ctx.skip_types = ["citations"] + + tool_source = get_xml_tool_source(CITATIONS_ABSENT) + run_lint_module(lint_ctx, citations, tool_source) + assert not lint_ctx.warn_messages + assert not lint_ctx.info_messages + assert not lint_ctx.valid_messages + assert not lint_ctx.error_messages + + def test_list_linters(): linter_names = Linter.list_listers() # make sure to add/remove a test for new/removed linters if this number changes @@ -2095,3 +2122,45 @@ def test_list_linters(): "XSD", ]: assert len([x for x in linter_names if x.startswith(prefix)]) + + +def test_linter_module_list(): + linter_modules = submodules.import_submodules(galaxy.tool_util.linters) + linter_module_names = [m.__name__.split(".")[-1] for m in linter_modules] + linter_module_names = [n for n in linter_module_names if not n.startswith("_")] + assert len(linter_module_names) >= 11 + + # until 23.2 the linters were implemented as functions lint_xyz contained in a module named xyz + # with 24.0 the functions were split in multiple classes (1 per linter message) + # in order to keep the skip functionality of lint contexts working (which is used eg in planemo) + # with the old names, we now also check for module name if a linter should be skipped + # therefore we test here that the module names are not changed + # the keys of the following dict represent the linter names before the switch and the values give + # the number of linter classes when we switched + # so adding new functionality to a linter module is fine. But splitting one or removing functionality + # should raise an error here to point to the possible consequences of renaming + old_linters = { + "citations": 4, + "command": 5, + "cwl": 9, + "general": 17, + "help": 6, + "inputs": 52, + "output": 11, + "stdio": 3, + "tests": 21, + "xml_order": 1, + } + assert len(set(linter_module_names).intersection(set(old_linters.keys()))) == len(old_linters.keys()) + + for module in linter_modules: + module_name = module.__name__.split(".")[-1] + if module_name not in old_linters: + continue + linter_cnt = 0 + for name, value in inspect.getmembers(module): + if callable(value) and name.startswith("lint_"): + continue + elif inspect.isclass(value) and issubclass(value, Linter) and not inspect.isabstract(value): + linter_cnt += 1 + assert linter_cnt >= old_linters[module_name]