From 997d2741c2ec1af7dc16bd9b318a2ccb98abcb39 Mon Sep 17 00:00:00 2001 From: mashehu Date: Fri, 16 Feb 2024 14:47:04 +0100 Subject: [PATCH 1/4] handle multiple module imports in linting closes #2726 --- nf_core/subworkflows/lint/main_nf.py | 24 +++++--- tests/subworkflows/lint.py | 85 ++++++++++++++++++++++++++++ tests/test_subworkflows.py | 3 + 3 files changed, 104 insertions(+), 8 deletions(-) diff --git a/nf_core/subworkflows/lint/main_nf.py b/nf_core/subworkflows/lint/main_nf.py index f59e1e427..c73559502 100644 --- a/nf_core/subworkflows/lint/main_nf.py +++ b/nf_core/subworkflows/lint/main_nf.py @@ -4,6 +4,7 @@ import logging import re +from typing import List log = logging.getLogger(__name__) @@ -120,7 +121,7 @@ def check_main_section(self, lines, included_components): # Check that all included components are used # Check that all included component versions are used - if included_components is not None: + if included_components: for component in included_components: if component in script: self.passed.append( @@ -152,7 +153,7 @@ def check_main_section(self, lines, included_components): ) -def check_subworkflow_section(self, lines): +def check_subworkflow_section(self, lines: List[str]) -> List[str]: """Lint the section of a subworkflow before the workflow definition Specifically checks if the subworkflow includes at least two modules or subworkflows @@ -160,7 +161,7 @@ def check_subworkflow_section(self, lines): lines (List[str]): Content of subworkflow. Returns: - List: List of included component names. If subworkflow doesn't contain any lines, return None. + List[str]: List of included components. """ # Check that we have subworkflow content if len(lines) == 0: @@ -171,7 +172,7 @@ def check_subworkflow_section(self, lines): self.main_nf, ) ) - return + return [] self.passed.append( ("subworkflow_include", "Subworkflow does include modules before the workflow definition", self.main_nf) ) @@ -179,10 +180,17 @@ def check_subworkflow_section(self, lines): includes = [] for line in lines: if line.strip().startswith("include"): - component_name = line.split("{")[1].split("}")[0].strip() - if " as " in component_name: - component_name = component_name.split(" as ")[1].strip() - includes.append(component_name) + component_name = [line.split("{")[1].split("}")[0].strip()] + # check if multiple components are included + if ";" in component_name[0]: + component_name = component_name[0].split(";") + for comp in component_name: + if " as " in comp: + comp = comp.split(" as ")[1].strip() + includes.append(comp) + continue + # remove duplicated components + includes = list(set(includes)) if len(includes) >= 2: self.passed.append(("main_nf_include", "Subworkflow includes two or more modules", self.main_nf)) else: diff --git a/tests/subworkflows/lint.py b/tests/subworkflows/lint.py index 8804f8bf6..8c7287075 100644 --- a/tests/subworkflows/lint.py +++ b/tests/subworkflows/lint.py @@ -104,3 +104,88 @@ def test_subworkflows_lint_snapshot_file_not_needed(self): assert len(subworkflow_lint.failed) == 0, f"Linting failed with {[x.__dict__ for x in subworkflow_lint.failed]}" assert len(subworkflow_lint.passed) > 0 assert len(subworkflow_lint.warned) >= 0 + + +def test_subworkflows_lint_less_than_two_modules_warning(self): + """Test linting a subworkflow with less than two modules""" + self.subworkflow_install.install("bam_stats_samtools") + # Remove two modules + with open(Path(self.pipeline_dir, "subworkflows", "nf-core", "bam_stats_samtools", "main.nf")) as fh: + content = fh.read() + new_content = content.replace( + "include { SAMTOOLS_IDXSTATS } from '../../../modules/nf-core/samtools/idxstats/main'", "" + ) + new_content = new_content.replace( + "include { SAMTOOLS_FLAGSTAT } from '../../../modules/nf-core/samtools/flagstat/main'", "" + ) + with open(Path(self.pipeline_dir, "subworkflows", "nf-core", "bam_stats_samtools", "main.nf"), "w") as fh: + fh.write(new_content) + subworkflow_lint = nf_core.subworkflows.SubworkflowLint(dir=self.pipeline_dir) + subworkflow_lint.lint(print_results=False, subworkflow="bam_stats_samtools") + assert len(subworkflow_lint.failed) >= 0, f"Linting failed with {[x.__dict__ for x in subworkflow_lint.failed]}" + assert len(subworkflow_lint.passed) > 0 + assert len(subworkflow_lint.warned) > 0 + assert subworkflow_lint.warned[0].lint_test == "main_nf_include" + # cleanup + self.subworkflow_remove.remove("bam_stats_samtools", force=True) + + +def test_subworkflows_lint_include_multiple_alias(self): + """Test linting a subworkflow with multiple include methods""" + self.subworkflow_install.install("bam_stats_samtools") + with open(Path(self.pipeline_dir, "subworkflows", "nf-core", "bam_stats_samtools", "main.nf")) as fh: + content = fh.read() + new_content = content.replace( + "include { SAMTOOLS_STATS }", + "include { SAMTOOLS_STATS as SAMTOOLS_STATS_1; SAMTOOLS_STATS as SAMTOOLS_STATS_2 }", + ) + with open(Path(self.pipeline_dir, "subworkflows", "nf-core", "bam_stats_samtools", "main.nf"), "w") as fh: + fh.write(new_content) + + subworkflow_lint = nf_core.subworkflows.SubworkflowLint(dir=self.pipeline_dir) + subworkflow_lint.lint(print_results=False, subworkflow="bam_stats_samtools") + assert len(subworkflow_lint.failed) >= 0, f"Linting failed with {[x.__dict__ for x in subworkflow_lint.failed]}" + assert len(subworkflow_lint.passed) > 0 + assert len(subworkflow_lint.warned) == 0 + # assert that one message mentions SAMTOOLS_STATS but not SAMTOOLS_STATS_1 or SAMTOOLS_STATS_2 + assert any( + [ + x.message == "Included component 'SAMTOOLS_STATS' versions are added in main.nf" + for x in subworkflow_lint.passed + ] + ) + assert not any( + [ + x.message == "Included component 'SAMTOOLS_STATS_1' versions are added in main.nf" + for x in subworkflow_lint.passed + ] + ) + assert not any( + [ + x.message == "Included component 'SAMTOOLS_STATS_2' versions are added in main.nf" + for x in subworkflow_lint.passed + ] + ) + + # cleanup + self.subworkflow_remove.remove("bam_stats_samtools", force=True) + + +def test_subworkflows_lint_capitalization_fail(self): + """Test linting a subworkflow with a capitalization fail""" + self.subworkflow_install.install("bam_stats_samtools") + # change workflow name to lowercase + with open(Path(self.pipeline_dir, "subworkflows", "nf-core", "bam_stats_samtools", "main.nf")) as fh: + content = fh.read() + new_content = content.replace("workflow BAM_STATS_SAMTOOLS {", "workflow bam_stats_samtools {") + with open(Path(self.pipeline_dir, "subworkflows", "nf-core", "bam_stats_samtools", "main.nf"), "w") as fh: + fh.write(new_content) + subworkflow_lint = nf_core.subworkflows.SubworkflowLint(dir=self.pipeline_dir) + subworkflow_lint.lint(print_results=False, subworkflow="bam_stats_samtools") + assert len(subworkflow_lint.failed) >= 1, f"Linting failed with {[x.__dict__ for x in subworkflow_lint.failed]}" + assert len(subworkflow_lint.passed) > 0 + assert len(subworkflow_lint.warned) >= 0 + assert any([x.lint_test == "workflow_capitals" for x in subworkflow_lint.failed]) + + # cleanup + self.subworkflow_remove.remove("bam_stats_samtools", force=True) diff --git a/tests/test_subworkflows.py b/tests/test_subworkflows.py index 19872ee16..cd0bd2114 100644 --- a/tests/test_subworkflows.py +++ b/tests/test_subworkflows.py @@ -121,8 +121,11 @@ def tearDown(self): ) from .subworkflows.lint import ( # type: ignore[misc] test_subworkflows_lint, + test_subworkflows_lint_capitalization_fail, test_subworkflows_lint_empty, test_subworkflows_lint_gitlab_subworkflows, + test_subworkflows_lint_include_multiple_alias, + test_subworkflows_lint_less_than_two_modules_warning, test_subworkflows_lint_multiple_remotes, test_subworkflows_lint_new_subworkflow, test_subworkflows_lint_no_gitlab, From 27da6a609f904c54fba6df35c092424ece7954b1 Mon Sep 17 00:00:00 2001 From: nf-core-bot Date: Fri, 16 Feb 2024 13:50:17 +0000 Subject: [PATCH 2/4] [automated] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a37e6e5ff..856fd1198 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ ### Modules - Handle dirty local module repos by force checkout of commits and branches if needed ([#2734](https://github.com/nf-core/tools/pull/2734)) +- Linting: Handle multiple aliases in module imports correctly during linting ([#2762](https://github.com/nf-core/tools/pull/2762)) ### General From 243ec6437efa1bd58724f482c50a1a3d15df1b7b Mon Sep 17 00:00:00 2001 From: mashehu Date: Fri, 16 Feb 2024 14:51:35 +0100 Subject: [PATCH 3/4] fix changelog --- CHANGELOG.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 856fd1198..6f349101d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,13 +11,13 @@ ### Linting -- make creat-lint-wf composable ([#2733](https://github.com/nf-core/tools/pull/2733)) -- add looser comparison when pipeline logos ([#2744](https://github.com/nf-core/tools/pull/2744)) +- Make creat-lint-wf composable ([#2733](https://github.com/nf-core/tools/pull/2733)) +- Add looser comparison when pipeline logos ([#2744](https://github.com/nf-core/tools/pull/2744)) +- Handle multiple aliases in module imports correctly during linting ([#2762](https://github.com/nf-core/tools/pull/2762)) ### Modules - Handle dirty local module repos by force checkout of commits and branches if needed ([#2734](https://github.com/nf-core/tools/pull/2734)) -- Linting: Handle multiple aliases in module imports correctly during linting ([#2762](https://github.com/nf-core/tools/pull/2762)) ### General From a84079050ba28d85e078e918a9a7e4b74f44ec38 Mon Sep 17 00:00:00 2001 From: mashehu Date: Fri, 16 Feb 2024 16:13:08 +0100 Subject: [PATCH 4/4] fix tests --- tests/subworkflows/lint.py | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/tests/subworkflows/lint.py b/tests/subworkflows/lint.py index 8c7287075..b89b7b78c 100644 --- a/tests/subworkflows/lint.py +++ b/tests/subworkflows/lint.py @@ -135,9 +135,10 @@ def test_subworkflows_lint_include_multiple_alias(self): self.subworkflow_install.install("bam_stats_samtools") with open(Path(self.pipeline_dir, "subworkflows", "nf-core", "bam_stats_samtools", "main.nf")) as fh: content = fh.read() - new_content = content.replace( - "include { SAMTOOLS_STATS }", - "include { SAMTOOLS_STATS as SAMTOOLS_STATS_1; SAMTOOLS_STATS as SAMTOOLS_STATS_2 }", + new_content = content.replace("SAMTOOLS_STATS", "SAMTOOLS_STATS_1") + new_content = new_content.replace( + "include { SAMTOOLS_STATS_1 ", + "include { SAMTOOLS_STATS as SAMTOOLS_STATS_1; SAMTOOLS_STATS as SAMTOOLS_STATS_2 ", ) with open(Path(self.pipeline_dir, "subworkflows", "nf-core", "bam_stats_samtools", "main.nf"), "w") as fh: fh.write(new_content) @@ -146,25 +147,16 @@ def test_subworkflows_lint_include_multiple_alias(self): subworkflow_lint.lint(print_results=False, subworkflow="bam_stats_samtools") assert len(subworkflow_lint.failed) >= 0, f"Linting failed with {[x.__dict__ for x in subworkflow_lint.failed]}" assert len(subworkflow_lint.passed) > 0 - assert len(subworkflow_lint.warned) == 0 - # assert that one message mentions SAMTOOLS_STATS but not SAMTOOLS_STATS_1 or SAMTOOLS_STATS_2 + assert len(subworkflow_lint.warned) == 2 assert any( - [ - x.message == "Included component 'SAMTOOLS_STATS' versions are added in main.nf" - for x in subworkflow_lint.passed - ] - ) - assert not any( [ x.message == "Included component 'SAMTOOLS_STATS_1' versions are added in main.nf" for x in subworkflow_lint.passed ] ) - assert not any( - [ - x.message == "Included component 'SAMTOOLS_STATS_2' versions are added in main.nf" - for x in subworkflow_lint.passed - ] + assert any([x.message == "Included component 'SAMTOOLS_STATS_1' used in main.nf" for x in subworkflow_lint.passed]) + assert any( + [x.message == "Included component 'SAMTOOLS_STATS_2' not used in main.nf" for x in subworkflow_lint.warned] ) # cleanup