From 46fcedf73ca429bc855174fe54e4a8a3faf8b12d Mon Sep 17 00:00:00 2001 From: Arthur Gymer <24782660+awgymer@users.noreply.github.com> Date: Wed, 29 Mar 2023 14:44:25 +0100 Subject: [PATCH 1/6] Lint all labels and check for conflicting or non-standard --- nf_core/modules/lint/main_nf.py | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/nf_core/modules/lint/main_nf.py b/nf_core/modules/lint/main_nf.py index d44fe90f1e..6a75667a75 100644 --- a/nf_core/modules/lint/main_nf.py +++ b/nf_core/modules/lint/main_nf.py @@ -236,25 +236,30 @@ def check_process_section(self, lines, fix_version, progress_bar): # Check that process labels are correct correct_process_labels = ["process_single", "process_low", "process_medium", "process_high", "process_long"] - process_label = [l for l in lines if l.lstrip().startswith("label")] - if len(process_label) > 0: - try: - process_label = re.search("process_[A-Za-z]+", process_label[0]).group(0) - except AttributeError: - process_label = re.search("'([A-Za-z_-]+)'", process_label[0]).group(0) - finally: - if not process_label in correct_process_labels: + all_labels = [l for l in lines if l.lstrip().startswith("label ")] + bad_labels = [] + good_labels = [] + if len(all_labels) > 0: + for label in all_labels: + label = re.match("^label\s+([a-zA-Z0-9_-]+)", label).group(1) + if label not in correct_process_labels: + bad_labels.append(label) + else: + good_labels.append(label) + if len(good_labels) > 1: self.warned.append( ( "process_standard_label", - f"Process label ({process_label}) is not among standard labels: `{'`,`'.join(correct_process_labels)}`", + f"Conflicting process labels found: `{'`,`'.join(good_labels)}`", self.main_nf, ) ) - else: + elif len(good_labels) == 1: self.passed.append(("process_standard_label", "Correct process label", self.main_nf)) + else: + self.warned.append(("process_standard_label", "Standard process label not found", self.main_nf)) else: - self.warned.append(("process_standard_label", "Process label unspecified", self.main_nf)) + self.warned.append(("process_standard_label", "Process label not specified", self.main_nf)) for i, l in enumerate(lines): url = None if _container_type(l) == "bioconda": From 7849dcccd64f789315fef61baece44b7c078da29 Mon Sep 17 00:00:00 2001 From: Arthur Gymer <24782660+awgymer@users.noreply.github.com> Date: Tue, 4 Apr 2023 12:13:25 +0100 Subject: [PATCH 2/6] Add checks for duplicated labels and warn if non-standard labels found --- nf_core/modules/lint/main_nf.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/nf_core/modules/lint/main_nf.py b/nf_core/modules/lint/main_nf.py index 6a75667a75..fbcc68202c 100644 --- a/nf_core/modules/lint/main_nf.py +++ b/nf_core/modules/lint/main_nf.py @@ -258,6 +258,19 @@ def check_process_section(self, lines, fix_version, progress_bar): self.passed.append(("process_standard_label", "Correct process label", self.main_nf)) else: self.warned.append(("process_standard_label", "Standard process label not found", self.main_nf)) + if len(bad_labels) > 0: + self.warned.append( + ("process_standard_label", f"Non-standard labels found: `{'`,`'.join(good_labels)}`", self.main_nf) + ) + if len(all_labels) > len(set(all_labels)): + self.warned.append( + ( + "process_standard_label", + f"Duplicate labels found: `{'`,`'.join(sorted(all_labels))}`", + self.main_nf, + ) + ) + else: self.warned.append(("process_standard_label", "Process label not specified", self.main_nf)) for i, l in enumerate(lines): From 2915f2ebd4353b4be5f67e4a1c1b0a98c55234d9 Mon Sep 17 00:00:00 2001 From: Arthur Gymer <24782660+awgymer@users.noreply.github.com> Date: Tue, 4 Apr 2023 12:15:34 +0100 Subject: [PATCH 3/6] Fix indentation so that warning logic is outside loop and not repeated for every label --- nf_core/modules/lint/main_nf.py | 44 ++++++++++++++++----------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/nf_core/modules/lint/main_nf.py b/nf_core/modules/lint/main_nf.py index fbcc68202c..b980b892af 100644 --- a/nf_core/modules/lint/main_nf.py +++ b/nf_core/modules/lint/main_nf.py @@ -246,30 +246,30 @@ def check_process_section(self, lines, fix_version, progress_bar): bad_labels.append(label) else: good_labels.append(label) - if len(good_labels) > 1: - self.warned.append( - ( - "process_standard_label", - f"Conflicting process labels found: `{'`,`'.join(good_labels)}`", - self.main_nf, - ) - ) - elif len(good_labels) == 1: - self.passed.append(("process_standard_label", "Correct process label", self.main_nf)) - else: - self.warned.append(("process_standard_label", "Standard process label not found", self.main_nf)) - if len(bad_labels) > 0: - self.warned.append( - ("process_standard_label", f"Non-standard labels found: `{'`,`'.join(good_labels)}`", self.main_nf) + if len(good_labels) > 1: + self.warned.append( + ( + "process_standard_label", + f"Conflicting process labels found: `{'`,`'.join(good_labels)}`", + self.main_nf, ) - if len(all_labels) > len(set(all_labels)): - self.warned.append( - ( - "process_standard_label", - f"Duplicate labels found: `{'`,`'.join(sorted(all_labels))}`", - self.main_nf, - ) + ) + elif len(good_labels) == 1: + self.passed.append(("process_standard_label", "Correct process label", self.main_nf)) + else: + self.warned.append(("process_standard_label", "Standard process label not found", self.main_nf)) + if len(bad_labels) > 0: + self.warned.append( + ("process_standard_label", f"Non-standard labels found: `{'`,`'.join(good_labels)}`", self.main_nf) + ) + if len(all_labels) > len(set(all_labels)): + self.warned.append( + ( + "process_standard_label", + f"Duplicate labels found: `{'`,`'.join(sorted(all_labels))}`", + self.main_nf, ) + ) else: self.warned.append(("process_standard_label", "Process label not specified", self.main_nf)) From c57dbf3d90cb64e841b9860cb1e2527763047569 Mon Sep 17 00:00:00 2001 From: Arthur Gymer <24782660+awgymer@users.noreply.github.com> Date: Tue, 4 Apr 2023 12:23:44 +0100 Subject: [PATCH 4/6] Add handling and warning for labels that do not match the expected regex --- nf_core/modules/lint/main_nf.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/nf_core/modules/lint/main_nf.py b/nf_core/modules/lint/main_nf.py index b980b892af..22fb6459d8 100644 --- a/nf_core/modules/lint/main_nf.py +++ b/nf_core/modules/lint/main_nf.py @@ -241,7 +241,17 @@ def check_process_section(self, lines, fix_version, progress_bar): good_labels = [] if len(all_labels) > 0: for label in all_labels: - label = re.match("^label\s+([a-zA-Z0-9_-]+)", label).group(1) + try: + label = re.match("^label\s+([a-zA-Z0-9_-]+)", label).group(1) + except AttributeError: + self.warned.append( + ( + "process_standard_label", + f"Specified label appears to contain non-alphanumerics: {label}", + self.main_nf, + ) + ) + continue if label not in correct_process_labels: bad_labels.append(label) else: From ee0b95bce155e09f8f162ab647877c988a173c42 Mon Sep 17 00:00:00 2001 From: Arthur Gymer <24782660+awgymer@users.noreply.github.com> Date: Tue, 25 Apr 2023 23:19:17 +0100 Subject: [PATCH 5/6] Move label linting code to its own method and add test for that method only --- nf_core/modules/lint/main_nf.py | 98 +++++++++++++++-------------- tests/modules/lint.py | 108 ++++++++++++++++++++++++++++++++ 2 files changed, 159 insertions(+), 47 deletions(-) diff --git a/nf_core/modules/lint/main_nf.py b/nf_core/modules/lint/main_nf.py index b5bd1c1172..8ea4d9863b 100644 --- a/nf_core/modules/lint/main_nf.py +++ b/nf_core/modules/lint/main_nf.py @@ -235,54 +235,8 @@ def check_process_section(self, lines, fix_version, progress_bar): self.failed.append(("process_capitals", "Process name is not in capital letters", self.main_nf)) # Check that process labels are correct - correct_process_labels = ["process_single", "process_low", "process_medium", "process_high", "process_long"] - all_labels = [l for l in lines if l.lstrip().startswith("label ")] - bad_labels = [] - good_labels = [] - if len(all_labels) > 0: - for label in all_labels: - try: - label = re.match("^label\s+([a-zA-Z0-9_-]+)", label).group(1) - except AttributeError: - self.warned.append( - ( - "process_standard_label", - f"Specified label appears to contain non-alphanumerics: {label}", - self.main_nf, - ) - ) - continue - if label not in correct_process_labels: - bad_labels.append(label) - else: - good_labels.append(label) - if len(good_labels) > 1: - self.warned.append( - ( - "process_standard_label", - f"Conflicting process labels found: `{'`,`'.join(good_labels)}`", - self.main_nf, - ) - ) - elif len(good_labels) == 1: - self.passed.append(("process_standard_label", "Correct process label", self.main_nf)) - else: - self.warned.append(("process_standard_label", "Standard process label not found", self.main_nf)) - if len(bad_labels) > 0: - self.warned.append( - ("process_standard_label", f"Non-standard labels found: `{'`,`'.join(good_labels)}`", self.main_nf) - ) - if len(all_labels) > len(set(all_labels)): - self.warned.append( - ( - "process_standard_label", - f"Duplicate labels found: `{'`,`'.join(sorted(all_labels))}`", - self.main_nf, - ) - ) - else: - self.warned.append(("process_standard_label", "Process label not specified", self.main_nf)) + # Deprecated enable_conda for i, l in enumerate(lines): url = None l = l.strip(" '\"") @@ -437,6 +391,56 @@ def check_process_section(self, lines, fix_version, progress_bar): return docker_tag == singularity_tag +def check_process_labels(self, lines): + correct_process_labels = ["process_single", "process_low", "process_medium", "process_high", "process_long"] + all_labels = [l for l in lines if l.lstrip().startswith("label ")] + bad_labels = [] + good_labels = [] + if len(all_labels) > 0: + for label in all_labels: + try: + label = re.match("^label\s+([a-zA-Z0-9_-]+)", label).group(1) + except AttributeError: + self.warned.append( + ( + "process_standard_label", + f"Specified label appears to contain non-alphanumerics: {label}", + self.main_nf, + ) + ) + continue + if label not in correct_process_labels: + bad_labels.append(label) + else: + good_labels.append(label) + if len(good_labels) > 1: + self.warned.append( + ( + "process_standard_label", + f"Conflicting process labels found: `{'`,`'.join(good_labels)}`", + self.main_nf, + ) + ) + elif len(good_labels) == 1: + self.passed.append(("process_standard_label", "Correct process label", self.main_nf)) + else: + self.warned.append(("process_standard_label", "Standard process label not found", self.main_nf)) + if len(bad_labels) > 0: + self.warned.append( + ("process_standard_label", f"Non-standard labels found: `{'`,`'.join(good_labels)}`", self.main_nf) + ) + if len(all_labels) > len(set(all_labels)): + self.warned.append( + ( + "process_standard_label", + f"Duplicate labels found: `{'`,`'.join(sorted(all_labels))}`", + self.main_nf, + ) + ) + else: + self.warned.append(("process_standard_label", "Process label not specified", self.main_nf)) + + def _parse_input(self, line_raw): """ Return list of input channel names from an input line. diff --git a/tests/modules/lint.py b/tests/modules/lint.py index 9bab9eddeb..193ca9a5b8 100644 --- a/tests/modules/lint.py +++ b/tests/modules/lint.py @@ -4,6 +4,7 @@ import pytest import nf_core.modules +from nf_core.modules.lint import main_nf from ..utils import GITLAB_URL, set_wd from .patch import BISMARK_ALIGN, CORRECT_SHA, PATCH_BRANCH, REPO_NAME, modify_main_nf @@ -105,3 +106,110 @@ def test_modules_lint_patched_modules(self): assert len(module_lint.failed) == 1 assert len(module_lint.passed) > 0 assert len(module_lint.warned) >= 0 + + +# A skeleton object with the passed/warned/failed list attrs +# Use this in place of a ModuleLint object to test behaviour of +# linting methods which don't need the full setup +class MockModuleLint: + def __init__(self): + self.passed = [] + self.warned = [] + self.failed = [] + + +PROCESS_LABEL_GOOD = ( + """ + label process_high + cpus 12 + """, + 1, + 0, + 0, +) +PROCESS_LABEL_NON_ALPHANUMERIC = ( + """ + label a:label:with:colons + cpus 12 + """, + 0, + 1, + 0, +) +PROCESS_LABEL_GOOD_CONFLICTING = ( + """ + label process_high + label process_low + cpus 12 + """, + 0, + 1, + 0, +) +PROCESS_LABEL_GOOD_DUPLICATES = ( + """ + label process_high + label process_high + cpus 12 + """, + 0, + 2, + 0, +) +PROCESS_LABEL_GOOD_AND_NONSTANDARD = ( + """ + label process_high + label process_extra_label + cpus 12 + """, + 1, + 2, + 0, +) +PROCESS_LABEL_NONSTANDARD = ( + """ + label process_extra_label + cpus 12 + """, + 0, + 1, + 0, +) +PROCESS_LABEL_NONSTANDARD_DUPLICATES = ( + """ + label process_extra_label + label process_extra_label + cpus 12 + """, + 0, + 3, + 0, +) +PROCESS_LABEL_NONE_FOUND = ( + """ + cpus 12 + """, + 0, + 1, + 0, +) + +PROCESS_LABEL_TEST_CASES = [ + PROCESS_LABEL_GOOD, + PROCESS_LABEL_NON_ALPHANUMERIC, + PROCESS_LABEL_GOOD_CONFLICTING, + PROCESS_LABEL_GOOD_DUPLICATES, + PROCESS_LABEL_GOOD_AND_NONSTANDARD, + PROCESS_LABEL_NONSTANDARD, + PROCESS_LABEL_NONSTANDARD_DUPLICATES, + PROCESS_LABEL_NONE_FOUND, +] + + +@pytest.mark.parametrize("lines,passed,warned,failed", PROCESS_LABEL_TEST_CASES) +def test_modules_lint_check_process_labels(self, lines, passed, warned, failed): + mocked_ModuleLint = MockModuleLint() + main_nf.check_process_labels(mocked_ModuleLint, lines) + assert len(mocked_ModuleLint.passed) == passed + assert len(mocked_ModuleLint.warned) == warned + assert len(mocked_ModuleLint.failed) == failed From d593f86f79fa64cfd2b24886fd08521e7573dce7 Mon Sep 17 00:00:00 2001 From: Arthur Gymer <24782660+awgymer@users.noreply.github.com> Date: Wed, 26 Apr 2023 10:22:02 +0100 Subject: [PATCH 6/6] Fix test cases and ensure test is run. Make sure label linting method is called --- nf_core/modules/lint/main_nf.py | 7 ++++--- tests/modules/lint.py | 23 +++++++++++++---------- tests/test_modules.py | 1 + 3 files changed, 18 insertions(+), 13 deletions(-) diff --git a/nf_core/modules/lint/main_nf.py b/nf_core/modules/lint/main_nf.py index 8ea4d9863b..e9e18b3e12 100644 --- a/nf_core/modules/lint/main_nf.py +++ b/nf_core/modules/lint/main_nf.py @@ -235,6 +235,7 @@ def check_process_section(self, lines, fix_version, progress_bar): self.failed.append(("process_capitals", "Process name is not in capital letters", self.main_nf)) # Check that process labels are correct + check_process_labels(self, lines) # Deprecated enable_conda for i, l in enumerate(lines): @@ -393,13 +394,13 @@ def check_process_section(self, lines, fix_version, progress_bar): def check_process_labels(self, lines): correct_process_labels = ["process_single", "process_low", "process_medium", "process_high", "process_long"] - all_labels = [l for l in lines if l.lstrip().startswith("label ")] + all_labels = [l.strip() for l in lines if l.lstrip().startswith("label ")] bad_labels = [] good_labels = [] if len(all_labels) > 0: for label in all_labels: try: - label = re.match("^label\s+([a-zA-Z0-9_-]+)", label).group(1) + label = re.match("^label\s+([a-zA-Z0-9_-]+)$", label).group(1) except AttributeError: self.warned.append( ( @@ -427,7 +428,7 @@ def check_process_labels(self, lines): self.warned.append(("process_standard_label", "Standard process label not found", self.main_nf)) if len(bad_labels) > 0: self.warned.append( - ("process_standard_label", f"Non-standard labels found: `{'`,`'.join(good_labels)}`", self.main_nf) + ("process_standard_label", f"Non-standard labels found: `{'`,`'.join(bad_labels)}`", self.main_nf) ) if len(all_labels) > len(set(all_labels)): self.warned.append( diff --git a/tests/modules/lint.py b/tests/modules/lint.py index 193ca9a5b8..d9bf0b9a78 100644 --- a/tests/modules/lint.py +++ b/tests/modules/lint.py @@ -117,6 +117,8 @@ def __init__(self): self.warned = [] self.failed = [] + self.main_nf = "main_nf" + PROCESS_LABEL_GOOD = ( """ @@ -133,7 +135,7 @@ def __init__(self): cpus 12 """, 0, - 1, + 2, 0, ) PROCESS_LABEL_GOOD_CONFLICTING = ( @@ -163,7 +165,7 @@ def __init__(self): cpus 12 """, 1, - 2, + 1, 0, ) PROCESS_LABEL_NONSTANDARD = ( @@ -172,7 +174,7 @@ def __init__(self): cpus 12 """, 0, - 1, + 2, 0, ) PROCESS_LABEL_NONSTANDARD_DUPLICATES = ( @@ -206,10 +208,11 @@ def __init__(self): ] -@pytest.mark.parametrize("lines,passed,warned,failed", PROCESS_LABEL_TEST_CASES) -def test_modules_lint_check_process_labels(self, lines, passed, warned, failed): - mocked_ModuleLint = MockModuleLint() - main_nf.check_process_labels(mocked_ModuleLint, lines) - assert len(mocked_ModuleLint.passed) == passed - assert len(mocked_ModuleLint.warned) == warned - assert len(mocked_ModuleLint.failed) == failed +def test_modules_lint_check_process_labels(self): + for test_case in PROCESS_LABEL_TEST_CASES: + process, passed, warned, failed = test_case + mocked_ModuleLint = MockModuleLint() + main_nf.check_process_labels(mocked_ModuleLint, process.splitlines()) + assert len(mocked_ModuleLint.passed) == passed + assert len(mocked_ModuleLint.warned) == warned + assert len(mocked_ModuleLint.failed) == failed diff --git a/tests/test_modules.py b/tests/test_modules.py index 74596822c1..31d34b1d1d 100644 --- a/tests/test_modules.py +++ b/tests/test_modules.py @@ -162,6 +162,7 @@ def test_modulesrepo_class(self): test_modules_install_trimgalore_twice, ) from .modules.lint import ( + test_modules_lint_check_process_labels, test_modules_lint_empty, test_modules_lint_gitlab_modules, test_modules_lint_multiple_remotes,