Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Linting: Handle multiple aliases in module imports correctly during linting #2762

Merged
merged 5 commits into from Feb 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Expand Up @@ -12,6 +12,9 @@

### 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))
- Handle multiple aliases in module imports correctly during linting ([#2762](https://github.com/nf-core/tools/pull/2762))
- 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))
- Switch to markdown based API and error docs ([#2758](https://github.com/nf-core/tools/pull/2758))
Expand Down
24 changes: 16 additions & 8 deletions nf_core/subworkflows/lint/main_nf.py
Expand Up @@ -4,6 +4,7 @@

import logging
import re
from typing import List

log = logging.getLogger(__name__)

Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -152,15 +153,15 @@ 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

Args:
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:
Expand All @@ -171,18 +172,25 @@ 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)
)

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:
Expand Down
77 changes: 77 additions & 0 deletions tests/subworkflows/lint.py
Expand Up @@ -104,3 +104,80 @@ 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("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)

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) == 2
assert any(
[
x.message == "Included component 'SAMTOOLS_STATS_1' 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
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)
3 changes: 3 additions & 0 deletions tests/test_subworkflows.py
Expand Up @@ -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,
Expand Down