From 5bc36e41ad319d4f869693294831dd45c18f77ff Mon Sep 17 00:00:00 2001 From: mashehu Date: Thu, 17 Oct 2024 13:54:29 +0200 Subject: [PATCH 01/21] allow mixed list and dict in lint config --- nf_core/utils.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nf_core/utils.py b/nf_core/utils.py index 87dd307e7..ff8da1eea 100644 --- a/nf_core/utils.py +++ b/nf_core/utils.py @@ -1088,7 +1088,7 @@ def get(self, item: str, default: Any = None) -> Any: return getattr(self, item, default) -LintConfigType = Optional[Dict[str, Union[List[str], List[Dict[str, List[str]]], bool]]] +LintConfigType = Optional[Dict[str, Union[List[str], List[Union[List[str], Dict[str, List[str]]]], bool]]] class NFCoreYamlConfig(BaseModel): @@ -1153,7 +1153,7 @@ def load_tools_config(directory: Union[str, Path] = ".") -> Tuple[Optional[Path] except ValidationError as e: error_message = f"Config file '{config_fn}' is invalid" for error in e.errors(): - error_message += f"\n{error['loc'][0]}: {error['msg']}" + error_message += f"\n{error['loc'][0]}: {error['msg']}\ninput: {error['input']}" raise AssertionError(error_message) wf_config = fetch_wf_config(Path(directory)) From 81bdb3b3587a0fe52339abc31720eaffe5898fcc Mon Sep 17 00:00:00 2001 From: mashehu Date: Thu, 17 Oct 2024 14:11:18 +0200 Subject: [PATCH 02/21] nested too deeply --- nf_core/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nf_core/utils.py b/nf_core/utils.py index ff8da1eea..4b6e2ddc7 100644 --- a/nf_core/utils.py +++ b/nf_core/utils.py @@ -1088,7 +1088,7 @@ def get(self, item: str, default: Any = None) -> Any: return getattr(self, item, default) -LintConfigType = Optional[Dict[str, Union[List[str], List[Union[List[str], Dict[str, List[str]]]], bool]]] +LintConfigType = Optional[Dict[str, Union[List[str], List[Union[str, Dict[str, List[str]]]], bool]]] class NFCoreYamlConfig(BaseModel): From 3d8d4b7de9cb1356511d61c346f02f43ffad5936 Mon Sep 17 00:00:00 2001 From: nf-core-bot Date: Thu, 17 Oct 2024 12:14:43 +0000 Subject: [PATCH 03/21] [automated] Update CHANGELOG.md --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 364a079a7..7a4fd3583 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,8 @@ ### Linting +- allow mixed str and dict in lint config ([#3228](https://github.com/nf-core/tools/pull/3228)) + ### Modules ### Subworkflows From 2b4029b699471e73114a4bd4c9da930c8259d55d Mon Sep 17 00:00:00 2001 From: mashehu Date: Mon, 21 Oct 2024 13:51:55 +0200 Subject: [PATCH 04/21] handle new nf-core.yml structure --- nf_core/pipelines/lint/files_exist.py | 2 ++ nf_core/pipelines/lint/files_unchanged.py | 2 ++ nf_core/pipelines/lint/template_strings.py | 4 +++- 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/nf_core/pipelines/lint/files_exist.py b/nf_core/pipelines/lint/files_exist.py index 9dd307d8b..62af34845 100644 --- a/nf_core/pipelines/lint/files_exist.py +++ b/nf_core/pipelines/lint/files_exist.py @@ -200,6 +200,8 @@ def files_exist(self) -> Dict[str, List[str]]: # Remove files that should be ignored according to the linting config ignore_files = self.lint_config.get("files_exist", []) if self.lint_config is not None else [] + if ignore_files is None: + ignore_files = [] def pf(file_path: Union[str, Path]) -> Path: return Path(self.wf_path, file_path) diff --git a/nf_core/pipelines/lint/files_unchanged.py b/nf_core/pipelines/lint/files_unchanged.py index 300b3674b..2a0f8ffd3 100644 --- a/nf_core/pipelines/lint/files_unchanged.py +++ b/nf_core/pipelines/lint/files_unchanged.py @@ -144,6 +144,8 @@ def _tf(file_path: Union[str, Path]) -> Path: return Path(test_pipeline_dir, file_path) ignore_files = self.lint_config.get("files_unchanged", []) if self.lint_config is not None else [] + if ignore_files is None: + ignore_files = [] # Files that must be completely unchanged from template for files in files_exact: diff --git a/nf_core/pipelines/lint/template_strings.py b/nf_core/pipelines/lint/template_strings.py index 11c5e8251..0bf2ccbec 100644 --- a/nf_core/pipelines/lint/template_strings.py +++ b/nf_core/pipelines/lint/template_strings.py @@ -39,8 +39,10 @@ def template_strings(self): ignored = [] # Files that should be ignored according to the linting config ignore_files = self.lint_config.get("template_strings", []) if self.lint_config is not None else [] - files = self.list_files() + if ignore_files is None: + ignore_files = [] + files = self.list_files() # Loop through files, searching for string num_matches = 0 for fn in files: From 3a55f3682dde79066b3148e51388d80249a19de0 Mon Sep 17 00:00:00 2001 From: mashehu Date: Mon, 21 Oct 2024 13:52:38 +0200 Subject: [PATCH 05/21] update documentation for `multiqc_config` linting --- nf_core/pipelines/lint/multiqc_config.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/nf_core/pipelines/lint/multiqc_config.py b/nf_core/pipelines/lint/multiqc_config.py index 2b0fc7902..fec5b518e 100644 --- a/nf_core/pipelines/lint/multiqc_config.py +++ b/nf_core/pipelines/lint/multiqc_config.py @@ -31,6 +31,15 @@ def multiqc_config(self) -> Dict[str, List[str]]: lint: multiqc_config: False + To disable this test only for specific sections, you can specify a list of section names. + For example: + + .. code-block:: yaml + lint: + multiqc_config: + - report_section_order + - report_comment + """ passed: List[str] = [] From fcc442aae3019facd0a2a5b84397d08cbe503c0e Mon Sep 17 00:00:00 2001 From: mashehu Date: Mon, 21 Oct 2024 13:53:17 +0200 Subject: [PATCH 06/21] parse yaml correctly --- nf_core/pipelines/lint/nfcore_yml.py | 31 +++++++------- nf_core/utils.py | 62 +++++++++++++++++++++++++++- 2 files changed, 77 insertions(+), 16 deletions(-) diff --git a/nf_core/pipelines/lint/nfcore_yml.py b/nf_core/pipelines/lint/nfcore_yml.py index e0d5fb200..3395696d1 100644 --- a/nf_core/pipelines/lint/nfcore_yml.py +++ b/nf_core/pipelines/lint/nfcore_yml.py @@ -1,7 +1,8 @@ -import re from pathlib import Path from typing import Dict, List +from ruamel.yaml import YAML + from nf_core import __version__ REPOSITORY_TYPES = ["pipeline", "modules"] @@ -26,21 +27,23 @@ def nfcore_yml(self) -> Dict[str, List[str]]: failed: List[str] = [] ignored: List[str] = [] + yaml = YAML() + # Remove field that should be ignored according to the linting config ignore_configs = self.lint_config.get(".nf-core", []) if self.lint_config is not None else [] - try: - with open(Path(self.wf_path, ".nf-core.yml")) as fh: - content = fh.read() - except FileNotFoundError: - with open(Path(self.wf_path, ".nf-core.yaml")) as fh: - content = fh.read() + for ext in (".yml", ".yaml"): + try: + nf_core_yml = yaml.load(Path(self.wf_path) / f".nf-core{ext}") + break + except FileNotFoundError: + continue + else: + raise FileNotFoundError("No `.nf-core.yml` file found.") if "repository_type" not in ignore_configs: # Check that the repository type is set in the .nf-core.yml - repo_type_re = r"repository_type: (.+)" - match = re.search(repo_type_re, content) - if match: - repo_type = match.group(1) + if "repository_type" in nf_core_yml: + repo_type = nf_core_yml["repository_type"] if repo_type not in REPOSITORY_TYPES: failed.append( f"Repository type in `.nf-core.yml` is not valid. " @@ -55,10 +58,8 @@ def nfcore_yml(self) -> Dict[str, List[str]]: if "nf_core_version" not in ignore_configs: # Check that the nf-core version is set in the .nf-core.yml - nf_core_version_re = r"nf_core_version: (.+)" - match = re.search(nf_core_version_re, content) - if match: - nf_core_version = match.group(1).strip('"') + if "nf_core_version" in nf_core_yml: + nf_core_version = nf_core_yml["nf_core_version"] if nf_core_version != __version__ and "dev" not in nf_core_version: warned.append( f"nf-core version in `.nf-core.yml` is not set to the latest version. " diff --git a/nf_core/utils.py b/nf_core/utils.py index 4b6e2ddc7..5cce2494c 100644 --- a/nf_core/utils.py +++ b/nf_core/utils.py @@ -1091,6 +1091,63 @@ def get(self, item: str, default: Any = None) -> Any: LintConfigType = Optional[Dict[str, Union[List[str], List[Union[str, Dict[str, List[str]]]], bool]]] +class NFCoreYamlLintConfig(BaseModel): + """ + schema for linting config in `.nf-core.yml` should cover: + + .. code-block:: yaml + files_unchanged: + - .github/workflows/branch.yml + modules_config: False + modules_config: + - fastqc + # merge_markers: False + merge_markers: + - docs/my_pdf.pdf + nextflow_config: False + nextflow_config: + - manifest.name + - config_defaults: + - params.annotation_db + - params.multiqc_comment_headers + - params.custom_table_headers + # multiqc_config: False + multiqc_config: + - report_section_order + - report_comment + files_exist: + - .github/CONTRIBUTING.md + - CITATIONS.md + template_strings: False + template_strings: + - docs/my_pdf.pdf + """ + + files_unchanged: Optional[List[str]] = None + """ List of files that should not be changed """ + modules_config: Optional[Union[bool, List[str]]] = None + """ List of modules that should not be changed """ + merge_markers: Optional[Union[bool, List[str]]] = None + """ List of files that should not contain merge markers """ + nextflow_config: Optional[Union[bool, List[Union[str, Dict[str, List[str]]]]]] = None + """ List of Nextflow config files that should not be changed """ + multiqc_config: Optional[List[str]] = None + """ List of MultiQC config options that be changed """ + files_exist: Optional[List[str]] = None + """ List of files that can not exist """ + template_strings: Optional[Union[bool, List[str]]] = None + """ List of files that can contain template strings """ + + def __getitem__(self, item: str) -> Any: + return getattr(self, item) + + def get(self, item: str, default: Any = None) -> Any: + return getattr(self, item, default) + + def __setitem__(self, item: str, value: Any) -> None: + setattr(self, item, value) + + class NFCoreYamlConfig(BaseModel): """.nf-core.yml configuration file schema""" @@ -1100,7 +1157,7 @@ class NFCoreYamlConfig(BaseModel): """ Version of nf-core/tools used to create/update the pipeline""" org_path: Optional[str] = None """ Path to the organisation's modules repository (used for modules repo_type only) """ - lint: Optional[LintConfigType] = None + lint: Optional[NFCoreYamlLintConfig] = None """ Pipeline linting configuration, see https://nf-co.re/docs/nf-core-tools/pipelines/lint#linting-config for examples and documentation """ template: Optional[NFCoreTemplateConfig] = None """ Pipeline template configuration """ @@ -1115,6 +1172,9 @@ def __getitem__(self, item: str) -> Any: def get(self, item: str, default: Any = None) -> Any: return getattr(self, item, default) + def __setitem__(self, item: str, value: Any) -> None: + setattr(self, item, value) + def load_tools_config(directory: Union[str, Path] = ".") -> Tuple[Optional[Path], Optional[NFCoreYamlConfig]]: """ From 482f9f9e3f7b4ad1864af20a03078335f77ab12a Mon Sep 17 00:00:00 2001 From: mashehu Date: Mon, 21 Oct 2024 14:12:28 +0200 Subject: [PATCH 07/21] found a better way to handle the ignore file being None --- nf_core/pipelines/lint/files_exist.py | 2 -- nf_core/pipelines/lint/files_unchanged.py | 2 -- nf_core/pipelines/lint/template_strings.py | 2 -- nf_core/utils.py | 8 +++++--- 4 files changed, 5 insertions(+), 9 deletions(-) diff --git a/nf_core/pipelines/lint/files_exist.py b/nf_core/pipelines/lint/files_exist.py index 62af34845..9dd307d8b 100644 --- a/nf_core/pipelines/lint/files_exist.py +++ b/nf_core/pipelines/lint/files_exist.py @@ -200,8 +200,6 @@ def files_exist(self) -> Dict[str, List[str]]: # Remove files that should be ignored according to the linting config ignore_files = self.lint_config.get("files_exist", []) if self.lint_config is not None else [] - if ignore_files is None: - ignore_files = [] def pf(file_path: Union[str, Path]) -> Path: return Path(self.wf_path, file_path) diff --git a/nf_core/pipelines/lint/files_unchanged.py b/nf_core/pipelines/lint/files_unchanged.py index 2a0f8ffd3..300b3674b 100644 --- a/nf_core/pipelines/lint/files_unchanged.py +++ b/nf_core/pipelines/lint/files_unchanged.py @@ -144,8 +144,6 @@ def _tf(file_path: Union[str, Path]) -> Path: return Path(test_pipeline_dir, file_path) ignore_files = self.lint_config.get("files_unchanged", []) if self.lint_config is not None else [] - if ignore_files is None: - ignore_files = [] # Files that must be completely unchanged from template for files in files_exact: diff --git a/nf_core/pipelines/lint/template_strings.py b/nf_core/pipelines/lint/template_strings.py index 0bf2ccbec..0cb669e55 100644 --- a/nf_core/pipelines/lint/template_strings.py +++ b/nf_core/pipelines/lint/template_strings.py @@ -39,8 +39,6 @@ def template_strings(self): ignored = [] # Files that should be ignored according to the linting config ignore_files = self.lint_config.get("template_strings", []) if self.lint_config is not None else [] - if ignore_files is None: - ignore_files = [] files = self.list_files() # Loop through files, searching for string diff --git a/nf_core/utils.py b/nf_core/utils.py index 5cce2494c..283e2e5c7 100644 --- a/nf_core/utils.py +++ b/nf_core/utils.py @@ -1123,7 +1123,7 @@ class NFCoreYamlLintConfig(BaseModel): - docs/my_pdf.pdf """ - files_unchanged: Optional[List[str]] = None + files_unchanged: List[str] = [] """ List of files that should not be changed """ modules_config: Optional[Union[bool, List[str]]] = None """ List of modules that should not be changed """ @@ -1131,12 +1131,14 @@ class NFCoreYamlLintConfig(BaseModel): """ List of files that should not contain merge markers """ nextflow_config: Optional[Union[bool, List[Union[str, Dict[str, List[str]]]]]] = None """ List of Nextflow config files that should not be changed """ - multiqc_config: Optional[List[str]] = None + multiqc_config: List[str] = [] """ List of MultiQC config options that be changed """ - files_exist: Optional[List[str]] = None + files_exist: List[str] = [] """ List of files that can not exist """ template_strings: Optional[Union[bool, List[str]]] = None """ List of files that can contain template strings """ + nfcore_components: Optional[bool] = None + """ Include all required files to use nf-core modules and subworkflows """ def __getitem__(self, item: str) -> Any: return getattr(self, item) From 663a9329bed700a484f7b86d1b501ebce9df7b9c Mon Sep 17 00:00:00 2001 From: mashehu Date: Mon, 21 Oct 2024 14:15:37 +0200 Subject: [PATCH 08/21] handle new lint config structure --- nf_core/pipelines/lint/__init__.py | 12 +++++------- nf_core/utils.py | 1 + 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/nf_core/pipelines/lint/__init__.py b/nf_core/pipelines/lint/__init__.py index 8cc7c37cb..82361565f 100644 --- a/nf_core/pipelines/lint/__init__.py +++ b/nf_core/pipelines/lint/__init__.py @@ -27,8 +27,8 @@ from nf_core import __version__ from nf_core.components.lint import ComponentLint from nf_core.pipelines.lint_utils import console +from nf_core.utils import NFCoreYamlConfig, NFCoreYamlLintConfig, strip_ansi_codes from nf_core.utils import plural_s as _s -from nf_core.utils import strip_ansi_codes from .actions_awsfulltest import actions_awsfulltest from .actions_awstest import actions_awstest @@ -112,7 +112,7 @@ def __init__( # Initialise the parent object super().__init__(wf_path) - self.lint_config = {} + self.lint_config: Optional[NFCoreYamlLintConfig] = None self.release_mode = release_mode self.fail_ignored = fail_ignored self.fail_warned = fail_warned @@ -173,12 +173,11 @@ def _load_lint_config(self) -> bool: Add parsed config to the `self.lint_config` class attribute. """ _, tools_config = nf_core.utils.load_tools_config(self.wf_path) - self.lint_config = getattr(tools_config, "lint", {}) or {} + self.lint_config = getattr(tools_config, "lint", None) or None is_correct = True - # Check if we have any keys that don't match lint test names if self.lint_config is not None: - for k in self.lint_config: + for k, v in self.lint_config: if k != "nfcore_components" and k not in self.lint_tests: # nfcore_components is an exception to allow custom pipelines without nf-core components log.warning(f"Found unrecognised test name '{k}' in pipeline lint config") @@ -594,7 +593,7 @@ def run_linting( lint_obj._load_lint_config() lint_obj.load_pipeline_config() - if "nfcore_components" in lint_obj.lint_config and not lint_obj.lint_config["nfcore_components"]: + if lint_obj.lint_config and lint_obj.lint_config["nfcore_components"] is False: module_lint_obj = None subworkflow_lint_obj = None else: @@ -679,5 +678,4 @@ def run_linting( if len(lint_obj.failed) > 0: if release_mode: log.info("Reminder: Lint tests were run in --release mode.") - return lint_obj, module_lint_obj, subworkflow_lint_obj diff --git a/nf_core/utils.py b/nf_core/utils.py index 283e2e5c7..c3eb91987 100644 --- a/nf_core/utils.py +++ b/nf_core/utils.py @@ -1121,6 +1121,7 @@ class NFCoreYamlLintConfig(BaseModel): template_strings: False template_strings: - docs/my_pdf.pdf + nfcore_components: False """ files_unchanged: List[str] = [] From 53ae873e615478516e30c999c05338b8a5244823 Mon Sep 17 00:00:00 2001 From: mashehu Date: Mon, 21 Oct 2024 17:05:41 +0200 Subject: [PATCH 09/21] add tests with different valid yaml structures --- nf_core/utils.py | 8 +- tests/pipelines/lint/test_files_exist.py | 82 ++++++++++--- tests/pipelines/lint/test_nextflow_config.py | 20 ++-- tests/pipelines/lint/test_nfcore_yml.py | 112 ++++++++++++++---- tests/pipelines/lint/test_template_strings.py | 28 ++++- 5 files changed, 196 insertions(+), 54 deletions(-) diff --git a/nf_core/utils.py b/nf_core/utils.py index c3eb91987..03112dd1d 100644 --- a/nf_core/utils.py +++ b/nf_core/utils.py @@ -1126,17 +1126,17 @@ class NFCoreYamlLintConfig(BaseModel): files_unchanged: List[str] = [] """ List of files that should not be changed """ - modules_config: Optional[Union[bool, List[str]]] = None + modules_config: Optional[Union[bool, List[str]]] = [] """ List of modules that should not be changed """ - merge_markers: Optional[Union[bool, List[str]]] = None + merge_markers: Optional[Union[bool, List[str]]] = [] """ List of files that should not contain merge markers """ - nextflow_config: Optional[Union[bool, List[Union[str, Dict[str, List[str]]]]]] = None + nextflow_config: Optional[Union[bool, List[Union[str, Dict[str, List[str]]]]]] = [] """ List of Nextflow config files that should not be changed """ multiqc_config: List[str] = [] """ List of MultiQC config options that be changed """ files_exist: List[str] = [] """ List of files that can not exist """ - template_strings: Optional[Union[bool, List[str]]] = None + template_strings: Optional[Union[bool, List[str]]] = [] """ List of files that can contain template strings """ nfcore_components: Optional[bool] = None """ Include all required files to use nf-core modules and subworkflows """ diff --git a/tests/pipelines/lint/test_files_exist.py b/tests/pipelines/lint/test_files_exist.py index 97dd346cd..eb1ba9a17 100644 --- a/tests/pipelines/lint/test_files_exist.py +++ b/tests/pipelines/lint/test_files_exist.py @@ -1,5 +1,7 @@ from pathlib import Path +from ruamel.yaml import YAML + import nf_core.pipelines.lint from ..test_lint import TestLint @@ -9,17 +11,17 @@ class TestLintFilesExist(TestLint): def setUp(self) -> None: super().setUp() self.new_pipeline = self._make_pipeline_copy() + self.lint_obj = nf_core.pipelines.lint.PipelineLint(self.new_pipeline) def test_files_exist_missing_config(self): """Lint test: critical files missing FAIL""" Path(self.new_pipeline, "CHANGELOG.md").unlink() - lint_obj = nf_core.pipelines.lint.PipelineLint(self.new_pipeline) - lint_obj._load() - lint_obj.nf_config["manifest.name"] = "nf-core/testpipeline" + assert self.lint_obj._load() + self.lint_obj.nf_config["manifest.name"] = "nf-core/testpipeline" - results = lint_obj.files_exist() + results = self.lint_obj.files_exist() assert "File not found: `CHANGELOG.md`" in results["failed"] def test_files_exist_missing_main(self): @@ -27,10 +29,9 @@ def test_files_exist_missing_main(self): Path(self.new_pipeline, "main.nf").unlink() - lint_obj = nf_core.pipelines.lint.PipelineLint(self.new_pipeline) - lint_obj._load() + assert self.lint_obj._load() - results = lint_obj.files_exist() + results = self.lint_obj.files_exist() assert "File not found: `main.nf`" in results["warned"] def test_files_exist_deprecated_file(self): @@ -39,19 +40,17 @@ def test_files_exist_deprecated_file(self): nf = Path(self.new_pipeline, "parameters.settings.json") nf.touch() - lint_obj = nf_core.pipelines.lint.PipelineLint(self.new_pipeline) - lint_obj._load() + assert self.lint_obj._load() - results = lint_obj.files_exist() + results = self.lint_obj.files_exist() assert results["failed"] == ["File must be removed: `parameters.settings.json`"] def test_files_exist_pass(self): """Lint check should pass if all files are there""" - lint_obj = nf_core.pipelines.lint.PipelineLint(self.new_pipeline) - lint_obj._load() + assert self.lint_obj._load() - results = lint_obj.files_exist() + results = self.lint_obj.files_exist() assert results["failed"] == [] def test_files_exist_pass_conditional_nfschema(self): @@ -62,9 +61,58 @@ def test_files_exist_pass_conditional_nfschema(self): with open(Path(self.new_pipeline, "nextflow.config"), "w") as f: f.write(config) - lint_obj = nf_core.pipelines.lint.PipelineLint(self.new_pipeline) - lint_obj._load() - lint_obj.nf_config["manifest.schema"] = "nf-core" - results = lint_obj.files_exist() + assert self.lint_obj._load() + self.lint_obj.nf_config["manifest.schema"] = "nf-core" + results = self.lint_obj.files_exist() assert results["failed"] == [] assert results["ignored"] == [] + + def test_files_exists_pass_nf_core_yml_config(self): + """Check if linting passes with a valid nf-core.yml config""" + valid_yaml = """ + files_exist: + - .github/CONTRIBUTING.md + - CITATIONS.md + """ + yaml = YAML() + nf_core_yml_path = Path(self.new_pipeline, ".nf-core.yml") + nf_core_yml = yaml.load(nf_core_yml_path) + + nf_core_yml["lint"] = yaml.load(valid_yaml) + yaml.dump(nf_core_yml, nf_core_yml_path) + + self.lint_obj = nf_core.pipelines.lint.PipelineLint(self.new_pipeline) + assert self.lint_obj._load() + + results = self.lint_obj.files_exist() + assert results["failed"] == [] + assert "File is ignored: `.github/CONTRIBUTING.md`" in results["ignored"] + assert "File is ignored: `CITATIONS.md`" in results["ignored"] + + def test_files_exists_fail_nf_core_yml_config(self): + """Check if linting fails with a valid nf-core.yml config""" + valid_yaml = """ + files_exist: + - CITATIONS.md + """ + + # remove CITATIONS.md + Path(self.new_pipeline, "CITATIONS.md").unlink() + assert self.lint_obj._load() + # test first if linting fails correctly + results = self.lint_obj.files_exist() + assert "File not found: `CITATIONS.md`" in results["failed"] + + yaml = YAML() + nf_core_yml_path = Path(self.new_pipeline, ".nf-core.yml") + nf_core_yml = yaml.load(nf_core_yml_path) + + nf_core_yml["lint"] = yaml.load(valid_yaml) + yaml.dump(nf_core_yml, nf_core_yml_path) + + self.lint_obj = nf_core.pipelines.lint.PipelineLint(self.new_pipeline) + assert self.lint_obj._load() + + results = self.lint_obj.files_exist() + assert results["failed"] == [] + assert "File is ignored: `CITATIONS.md`" in results["ignored"] diff --git a/tests/pipelines/lint/test_nextflow_config.py b/tests/pipelines/lint/test_nextflow_config.py index a655fb8ac..f8c3c1f31 100644 --- a/tests/pipelines/lint/test_nextflow_config.py +++ b/tests/pipelines/lint/test_nextflow_config.py @@ -6,7 +6,6 @@ import nf_core.pipelines.create.create import nf_core.pipelines.lint -from nf_core.utils import NFCoreYamlConfig from ..test_lint import TestLint @@ -125,23 +124,30 @@ def test_allow_params_reference_in_main_nf(self): def test_default_values_ignored(self): """Test ignoring linting of default values.""" + valid_yaml = """ + nextflow_config: + - manifest.name + - config_defaults: + - params.custom_config_version + """ # Add custom_config_version to the ignore list nf_core_yml_path = Path(self.new_pipeline) / ".nf-core.yml" - nf_core_yml = NFCoreYamlConfig( - repository_type="pipeline", - lint={"nextflow_config": [{"config_defaults": ["params.custom_config_version"]}]}, - ) + + with open(nf_core_yml_path) as f: + nf_core_yml = yaml.safe_load(f) + nf_core_yml["lint"] = yaml.safe_load(valid_yaml) with open(nf_core_yml_path, "w") as f: - yaml.dump(nf_core_yml.model_dump(), f) + yaml.dump(nf_core_yml, f) lint_obj = nf_core.pipelines.lint.PipelineLint(self.new_pipeline) lint_obj.load_pipeline_config() lint_obj._load_lint_config() result = lint_obj.nextflow_config() assert len(result["failed"]) == 0 - assert len(result["ignored"]) == 1 + assert len(result["ignored"]) == 2 assert "Config default value correct: params.custom_config_version" not in str(result["passed"]) assert "Config default ignored: params.custom_config_version" in str(result["ignored"]) + assert "Config variable ignored: `manifest.name`" in str(result["ignored"]) def test_default_values_float(self): """Test comparing two float values.""" diff --git a/tests/pipelines/lint/test_nfcore_yml.py b/tests/pipelines/lint/test_nfcore_yml.py index 955c00da8..780e21241 100644 --- a/tests/pipelines/lint/test_nfcore_yml.py +++ b/tests/pipelines/lint/test_nfcore_yml.py @@ -1,8 +1,9 @@ -import re from pathlib import Path -import nf_core.pipelines.create +from ruamel.yaml import YAML + import nf_core.pipelines.lint +from nf_core.utils import NFCoreYamlConfig from ..test_lint import TestLint @@ -11,11 +12,14 @@ class TestLintNfCoreYml(TestLint): def setUp(self) -> None: super().setUp() self.new_pipeline = self._make_pipeline_copy() - self.nf_core_yml = Path(self.new_pipeline) / ".nf-core.yml" + self.nf_core_yml_path = Path(self.new_pipeline) / ".nf-core.yml" + self.yaml = YAML() + self.nf_core_yml: NFCoreYamlConfig = self.yaml.load(self.nf_core_yml_path) + self.lint_obj = nf_core.pipelines.lint.PipelineLint(self.new_pipeline) def test_nfcore_yml_pass(self): """Lint test: nfcore_yml - PASS""" - self.lint_obj._load() + assert self.lint_obj._load() results = self.lint_obj.nfcore_yml() assert "Repository type in `.nf-core.yml` is valid" in str(results["passed"]) @@ -27,14 +31,10 @@ def test_nfcore_yml_pass(self): def test_nfcore_yml_fail_repo_type(self): """Lint test: nfcore_yml - FAIL - repository type not set""" - with open(self.nf_core_yml) as fh: - content = fh.read() - new_content = content.replace("repository_type: pipeline", "repository_type: foo") - with open(self.nf_core_yml, "w") as fh: - fh.write(new_content) - lint_obj = nf_core.pipelines.lint.PipelineLint(self.new_pipeline) - lint_obj._load() - results = lint_obj.nfcore_yml() + self.nf_core_yml["repository_type"] = "foo" + self.yaml.dump(self.nf_core_yml, self.nf_core_yml_path) + assert self.lint_obj._load() + results = self.lint_obj.nfcore_yml() assert "Repository type in `.nf-core.yml` is not valid." in str(results["failed"]) assert len(results.get("warned", [])) == 0 assert len(results.get("passed", [])) >= 0 @@ -43,15 +43,87 @@ def test_nfcore_yml_fail_repo_type(self): def test_nfcore_yml_fail_nfcore_version(self): """Lint test: nfcore_yml - FAIL - nf-core version not set""" - with open(self.nf_core_yml) as fh: - content = fh.read() - new_content = re.sub(r"nf_core_version:.+", "nf_core_version: foo", content) - with open(self.nf_core_yml, "w") as fh: - fh.write(new_content) - lint_obj = nf_core.pipelines.lint.PipelineLint(self.new_pipeline) - lint_obj._load() - results = lint_obj.nfcore_yml() + self.nf_core_yml["nf_core_version"] = "foo" + self.yaml.dump(self.nf_core_yml, self.nf_core_yml_path) + assert self.lint_obj._load() + results = self.lint_obj.nfcore_yml() assert "nf-core version in `.nf-core.yml` is not set to the latest version." in str(results["warned"]) assert len(results.get("failed", [])) == 0 assert len(results.get("passed", [])) >= 0 assert len(results.get("ignored", [])) == 0 + + def test_nfcore_yml_nested_lint_config(self) -> None: + """Lint test: nfcore_yml with nested lint config - PASS""" + valid_yaml = """ + lint: + files_unchanged: + - .github/workflows/branch.yml + # modules_config: False + modules_config: + - fastqc + # merge_markers: False + merge_markers: + - docs/my_pdf.pdf + # nextflow_config: False + nextflow_config: + - manifest.name + - config_defaults: + - params.annotation_db + - params.multiqc_comment_headers + - params.custom_table_headers + multiqc_config: + - report_section_order + - report_comment + files_exist: + - .github/CONTRIBUTING.md + - CITATIONS.md + # template_strings: False + template_strings: + - docs/my_pdf.pdf + """ + self.nf_core_yml["lint"] = self.yaml.load(valid_yaml) + self.yaml.dump(self.nf_core_yml, self.nf_core_yml_path) + + assert self.lint_obj._load() + results = self.lint_obj.nfcore_yml() + assert len(results.get("failed", [])) == 0 + assert len(results.get("warned", [])) == 0 + assert len(results.get("ignored", [])) == 0 + + def test_nfcore_yml_nested_lint_config_bool(self) -> None: + """Lint test: nfcore_yml with nested lint config - PASS""" + valid_yaml = """ + lint: + files_unchanged: + - .github/workflows/branch.yml + modules_config: False + # modules_config: + # - fastqc + merge_markers: False + # merge_markers: + # - docs/my_pdf.pdf + # nextflow_config: False + nextflow_config: + - manifest.name + - config_defaults: + - params.annotation_db + - params.multiqc_comment_headers + - params.custom_table_headers + multiqc_config: + - report_section_order + - report_comment + files_exist: + - .github/CONTRIBUTING.md + - CITATIONS.md + template_strings: False + # template_strings: + # - docs/my_pdf.pdf + """ + self.nf_core_yml["lint"] = self.yaml.load(valid_yaml) + self.yaml.dump(self.nf_core_yml, self.nf_core_yml_path) + + assert self.lint_obj._load() + results = self.lint_obj.nfcore_yml() + assert len(results.get("failed", [])) == 0 + assert len(results.get("warned", [])) == 0 + assert len(results.get("ignored", [])) == 0 diff --git a/tests/pipelines/lint/test_template_strings.py b/tests/pipelines/lint/test_template_strings.py index 406ba63e0..37b760480 100644 --- a/tests/pipelines/lint/test_template_strings.py +++ b/tests/pipelines/lint/test_template_strings.py @@ -1,6 +1,8 @@ import subprocess from pathlib import Path +import yaml + import nf_core.pipelines.create import nf_core.pipelines.lint @@ -11,6 +13,9 @@ class TestLintTemplateStrings(TestLint): def setUp(self) -> None: super().setUp() self.new_pipeline = self._make_pipeline_copy() + self.nf_core_yml_path = Path(self.new_pipeline) / ".nf-core.yml" + with open(self.nf_core_yml_path) as f: + self.nf_core_yml = yaml.safe_load(f) def test_template_strings(self): """Tests finding a template string in a file fails linting.""" @@ -28,9 +33,12 @@ def test_template_strings(self): def test_template_strings_ignored(self): """Tests ignoring template_strings""" # Ignore template_strings test - nf_core_yml = Path(self.new_pipeline) / ".nf-core.yml" - with open(nf_core_yml, "w") as f: - f.write("repository_type: pipeline\nlint:\n template_strings: False") + valid_yaml = """ + template_strings: false + """ + self.nf_core_yml["lint"] = yaml.safe_load(valid_yaml) + with open(self.nf_core_yml_path, "w") as f: + yaml.safe_dump(self.nf_core_yml, f) lint_obj = nf_core.pipelines.lint.PipelineLint(self.new_pipeline) lint_obj._load() lint_obj._lint_pipeline() @@ -43,13 +51,21 @@ def test_template_strings_ignore_file(self): txt_file = Path(self.new_pipeline) / "docs" / "test.txt" with open(txt_file, "w") as f: f.write("my {{ template_string }}") + subprocess.check_output(["git", "add", "docs"], cwd=self.new_pipeline) + # Ignore template_strings test - nf_core_yml = Path(self.new_pipeline) / ".nf-core.yml" - with open(nf_core_yml, "w") as f: - f.write("repository_type: pipeline\nlint:\n template_strings:\n - docs/test.txt") + valid_yaml = """ + template_strings: + - docs/test.txt + """ + self.nf_core_yml["lint"] = yaml.safe_load(valid_yaml) + with open(self.nf_core_yml_path, "w") as f: + yaml.safe_dump(self.nf_core_yml, f) + lint_obj = nf_core.pipelines.lint.PipelineLint(self.new_pipeline) lint_obj._load() result = lint_obj.template_strings() + assert len(result["failed"]) == 0 assert len(result["ignored"]) == 1 From 57f7ca8680e5da788f04cb81cded4de4cbd0ad42 Mon Sep 17 00:00:00 2001 From: mashehu Date: Mon, 21 Oct 2024 17:33:31 +0200 Subject: [PATCH 10/21] remove last traces of LintConfigType --- nf_core/components/lint/__init__.py | 4 ++-- nf_core/pipelines/create/create.py | 6 +++--- nf_core/utils.py | 3 --- 3 files changed, 5 insertions(+), 8 deletions(-) diff --git a/nf_core/components/lint/__init__.py b/nf_core/components/lint/__init__.py index fcc3b414d..69740135a 100644 --- a/nf_core/components/lint/__init__.py +++ b/nf_core/components/lint/__init__.py @@ -22,7 +22,7 @@ from nf_core.components.nfcore_component import NFCoreComponent from nf_core.modules.modules_json import ModulesJson from nf_core.pipelines.lint_utils import console -from nf_core.utils import LintConfigType +from nf_core.utils import NFCoreYamlLintConfig from nf_core.utils import plural_s as _s log = logging.getLogger(__name__) @@ -80,7 +80,7 @@ def __init__( self.failed: List[LintResult] = [] self.all_local_components: List[NFCoreComponent] = [] - self.lint_config: Optional[LintConfigType] = None + self.lint_config: Optional[NFCoreYamlLintConfig] = None self.modules_json: Optional[ModulesJson] = None if self.component_type == "modules": diff --git a/nf_core/pipelines/create/create.py b/nf_core/pipelines/create/create.py index 8ab547c1c..98b2b704b 100644 --- a/nf_core/pipelines/create/create.py +++ b/nf_core/pipelines/create/create.py @@ -8,7 +8,7 @@ import re import shutil from pathlib import Path -from typing import Dict, List, Optional, Tuple, Union, cast +from typing import Dict, List, Optional, Tuple, Union import git import git.config @@ -21,7 +21,7 @@ from nf_core.pipelines.create.utils import CreateConfig, features_yml_path, load_features_yaml from nf_core.pipelines.create_logo import create_logo from nf_core.pipelines.lint_utils import run_prettier_on_file -from nf_core.utils import LintConfigType, NFCoreTemplateConfig +from nf_core.utils import NFCoreTemplateConfig, NFCoreYamlLintConfig log = logging.getLogger(__name__) @@ -395,7 +395,7 @@ def fix_linting(self): # Add the lint content to the preexisting nf-core config config_fn, nf_core_yml = nf_core.utils.load_tools_config(self.outdir) if config_fn is not None and nf_core_yml is not None: - nf_core_yml.lint = cast(LintConfigType, lint_config) + nf_core_yml.lint = NFCoreYamlLintConfig(**lint_config) with open(self.outdir / config_fn, "w") as fh: yaml.dump(nf_core_yml.model_dump(), fh, default_flow_style=False, sort_keys=False) diff --git a/nf_core/utils.py b/nf_core/utils.py index 03112dd1d..1b0d491e2 100644 --- a/nf_core/utils.py +++ b/nf_core/utils.py @@ -1088,9 +1088,6 @@ def get(self, item: str, default: Any = None) -> Any: return getattr(self, item, default) -LintConfigType = Optional[Dict[str, Union[List[str], List[Union[str, Dict[str, List[str]]]], bool]]] - - class NFCoreYamlLintConfig(BaseModel): """ schema for linting config in `.nf-core.yml` should cover: From e743185cf8d388bb18032aa9ebac5aca363d0da9 Mon Sep 17 00:00:00 2001 From: mashehu Date: Mon, 21 Oct 2024 17:48:58 +0200 Subject: [PATCH 11/21] fix incorrect type --- nf_core/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nf_core/utils.py b/nf_core/utils.py index 1b0d491e2..ac886755f 100644 --- a/nf_core/utils.py +++ b/nf_core/utils.py @@ -1129,7 +1129,7 @@ class NFCoreYamlLintConfig(BaseModel): """ List of files that should not contain merge markers """ nextflow_config: Optional[Union[bool, List[Union[str, Dict[str, List[str]]]]]] = [] """ List of Nextflow config files that should not be changed """ - multiqc_config: List[str] = [] + multiqc_config: Union[bool, List[str]] = [] """ List of MultiQC config options that be changed """ files_exist: List[str] = [] """ List of files that can not exist """ From 58869a18facf3571c84b6f51a4c4a877f25c251d Mon Sep 17 00:00:00 2001 From: mashehu Date: Mon, 21 Oct 2024 17:49:25 +0200 Subject: [PATCH 12/21] more type fixes --- nf_core/utils.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nf_core/utils.py b/nf_core/utils.py index ac886755f..4c4d9f73d 100644 --- a/nf_core/utils.py +++ b/nf_core/utils.py @@ -1121,7 +1121,7 @@ class NFCoreYamlLintConfig(BaseModel): nfcore_components: False """ - files_unchanged: List[str] = [] + files_unchanged: Union[bool, List[str]] = [] """ List of files that should not be changed """ modules_config: Optional[Union[bool, List[str]]] = [] """ List of modules that should not be changed """ @@ -1131,7 +1131,7 @@ class NFCoreYamlLintConfig(BaseModel): """ List of Nextflow config files that should not be changed """ multiqc_config: Union[bool, List[str]] = [] """ List of MultiQC config options that be changed """ - files_exist: List[str] = [] + files_exist: Union[bool, List[str]] = [] """ List of files that can not exist """ template_strings: Optional[Union[bool, List[str]]] = [] """ List of files that can contain template strings """ From afbd51b8c30b785cd49797434540b1fe2279ac1d Mon Sep 17 00:00:00 2001 From: mashehu Date: Tue, 22 Oct 2024 13:33:29 +0200 Subject: [PATCH 13/21] add all lint tests to config --- nf_core/utils.py | 38 +++++++++++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/nf_core/utils.py b/nf_core/utils.py index 4c4d9f73d..f7472ec94 100644 --- a/nf_core/utils.py +++ b/nf_core/utils.py @@ -1136,7 +1136,43 @@ class NFCoreYamlLintConfig(BaseModel): template_strings: Optional[Union[bool, List[str]]] = [] """ List of files that can contain template strings """ nfcore_components: Optional[bool] = None - """ Include all required files to use nf-core modules and subworkflows """ + """ Lint all required files to use nf-core modules and subworkflows """ + actions_ci: Optional[bool] = None + """ Lint all required files to use GitHub Actions CI """ + actions_awstest: Optional[bool] = None + """ Lint all required files to run tests on AWS """ + actions_awsfulltest: Optional[bool] = None + """ Lint all required files to run full tests on AWS """ + readme: Optional[bool] = None + """ Lint the README.md file """ + pipeline_todos: Optional[bool] = None + """ Lint for TODOs statements""" + plugin_includes: Optional[bool] = None + """ Lint for nextflow plugin """ + pipeline_name_conventions: Optional[bool] = None + """ Lint for pipeline name conventions """ + schema_lint: Optional[bool] = None + """ Lint nextflow_schema.json file""" + schema_params: Optional[bool] = None + """ Lint schema for all params """ + system_exit: Optional[bool] = None + """ Lint for System.exit calls in groovy/nextflow code """ + schema_description: Optional[bool] = None + """ Check that every parameter in the schema has a description. """ + actions_schema_validation: Optional[bool] = None + """ Lint GitHub Action workflow files with schema""" + modules_json: Optional[bool] = None + """ Lint modules.json file """ + modules_structure: Optional[bool] = None + """ Lint modules structure """ + base_config: Optional[bool] = None + """ Lint base.config file """ + nfcore_yml: Optional[bool] = None + """ Lint nf-core.yml """ + version_consistency: Optional[bool] = None + """ Lint for version consistency """ + included_configs: Optional[bool] = None + """ Lint for included configs """ def __getitem__(self, item: str) -> Any: return getattr(self, item) From 9bf91f51fe36ecbc9baa62122016f2fbda32d788 Mon Sep 17 00:00:00 2001 From: mashehu Date: Tue, 22 Oct 2024 14:14:07 +0200 Subject: [PATCH 14/21] switch all defaults to None and drop them on dump --- nf_core/pipelines/create/create.py | 10 +++++----- nf_core/pipelines/lint/readme.py | 15 +++++++++++++++ nf_core/pipelines/sync.py | 6 +++--- nf_core/utils.py | 20 +++++++++++--------- 4 files changed, 34 insertions(+), 17 deletions(-) diff --git a/nf_core/pipelines/create/create.py b/nf_core/pipelines/create/create.py index 98b2b704b..776fc8943 100644 --- a/nf_core/pipelines/create/create.py +++ b/nf_core/pipelines/create/create.py @@ -67,7 +67,7 @@ def __init__( _, config_yml = nf_core.utils.load_tools_config(outdir if outdir else Path().cwd()) # Obtain a CreateConfig object from `.nf-core.yml` config file if config_yml is not None and getattr(config_yml, "template", None) is not None: - self.config = CreateConfig(**config_yml["template"].model_dump()) + self.config = CreateConfig(**config_yml["template"].model_dump(exclude_none=True)) else: raise UserWarning("The template configuration was not provided in '.nf-core.yml'.") # Update the output directory @@ -205,7 +205,7 @@ def obtain_jinja_params_dict( config_yml = None # Set the parameters for the jinja template - jinja_params = self.config.model_dump() + jinja_params = self.config.model_dump(exclude_none=True) # Add template areas to jinja params and create list of areas with paths to skip skip_areas = [] @@ -363,8 +363,8 @@ def render_template(self) -> None: config_fn, config_yml = nf_core.utils.load_tools_config(self.outdir) if config_fn is not None and config_yml is not None: with open(str(config_fn), "w") as fh: - config_yml.template = NFCoreTemplateConfig(**self.config.model_dump()) - yaml.safe_dump(config_yml.model_dump(), fh) + config_yml.template = NFCoreTemplateConfig(**self.config.model_dump(exclude_none=True)) + yaml.safe_dump(config_yml.model_dump(exclude_none=True), fh) log.debug(f"Dumping pipeline template yml to pipeline config file '{config_fn.name}'") # Run prettier on files @@ -397,7 +397,7 @@ def fix_linting(self): if config_fn is not None and nf_core_yml is not None: nf_core_yml.lint = NFCoreYamlLintConfig(**lint_config) with open(self.outdir / config_fn, "w") as fh: - yaml.dump(nf_core_yml.model_dump(), fh, default_flow_style=False, sort_keys=False) + yaml.dump(nf_core_yml.model_dump(exclude_none=True), fh, default_flow_style=False, sort_keys=False) def make_pipeline_logo(self): """Fetch a logo for the new pipeline from the nf-core website""" diff --git a/nf_core/pipelines/lint/readme.py b/nf_core/pipelines/lint/readme.py index bdfad5200..5a10fbfce 100644 --- a/nf_core/pipelines/lint/readme.py +++ b/nf_core/pipelines/lint/readme.py @@ -23,6 +23,21 @@ def readme(self): * If pipeline is released but still contains a 'zenodo.XXXXXXX' tag, the test fails + To disable this test, add the following to the pipeline's ``.nf-core.yml`` file: + + .. code-block:: yaml + lint: + readme: False + + To disable subsets of these tests, add the following to the pipeline's ``.nf-core.yml`` file: + + .. code-block:: yaml + + lint: + readme: + nextflow_badge + zenodo_release + """ passed = [] warned = [] diff --git a/nf_core/pipelines/sync.py b/nf_core/pipelines/sync.py index 12b29f15e..896adda94 100644 --- a/nf_core/pipelines/sync.py +++ b/nf_core/pipelines/sync.py @@ -105,7 +105,7 @@ def __init__( with open(template_yaml_path) as f: self.config_yml.template = yaml.safe_load(f) with open(self.config_yml_path, "w") as fh: - yaml.safe_dump(self.config_yml.model_dump(), fh) + yaml.safe_dump(self.config_yml.model_dump(exclude_none=True), fh) log.info(f"Saved pipeline creation settings to '{self.config_yml_path}'") raise SystemExit( f"Please commit your changes and delete the {template_yaml_path} file. Then run the sync command again." @@ -271,7 +271,7 @@ def make_template_pipeline(self): self.config_yml.template.force = True with open(self.config_yml_path, "w") as config_path: - yaml.safe_dump(self.config_yml.model_dump(), config_path) + yaml.safe_dump(self.config_yml.model_dump(exclude_none=True), config_path) try: pipeline_create_obj = nf_core.pipelines.create.create.PipelineCreate( @@ -291,7 +291,7 @@ def make_template_pipeline(self): self.config_yml.template.outdir = "." # Update nf-core version self.config_yml.nf_core_version = nf_core.__version__ - dump_yaml_with_prettier(self.config_yml_path, self.config_yml.model_dump()) + dump_yaml_with_prettier(self.config_yml_path, self.config_yml.model_dump(exclude_none=True)) except Exception as err: # Reset to where you were to prevent git getting messed up. diff --git a/nf_core/utils.py b/nf_core/utils.py index f7472ec94..b31863435 100644 --- a/nf_core/utils.py +++ b/nf_core/utils.py @@ -1121,20 +1121,22 @@ class NFCoreYamlLintConfig(BaseModel): nfcore_components: False """ - files_unchanged: Union[bool, List[str]] = [] + files_unchanged: Optional[Union[bool, List[str]]] = None """ List of files that should not be changed """ - modules_config: Optional[Union[bool, List[str]]] = [] + modules_config: Optional[Optional[Union[bool, List[str]]]] = None """ List of modules that should not be changed """ - merge_markers: Optional[Union[bool, List[str]]] = [] + merge_markers: Optional[Optional[Union[bool, List[str]]]] = None """ List of files that should not contain merge markers """ - nextflow_config: Optional[Union[bool, List[Union[str, Dict[str, List[str]]]]]] = [] + nextflow_config: Optional[Optional[Union[bool, List[Union[str, Dict[str, List[str]]]]]]] = None """ List of Nextflow config files that should not be changed """ - multiqc_config: Union[bool, List[str]] = [] + multiqc_config: Optional[Union[bool, List[str]]] = None """ List of MultiQC config options that be changed """ - files_exist: Union[bool, List[str]] = [] + files_exist: Optional[Union[bool, List[str]]] = None """ List of files that can not exist """ - template_strings: Optional[Union[bool, List[str]]] = [] + template_strings: Optional[Optional[Union[bool, List[str]]]] = None """ List of files that can contain template strings """ + readme: Optional[Union[bool, List[str]]] = None + """ Lint the README.md file """ nfcore_components: Optional[bool] = None """ Lint all required files to use nf-core modules and subworkflows """ actions_ci: Optional[bool] = None @@ -1143,8 +1145,6 @@ class NFCoreYamlLintConfig(BaseModel): """ Lint all required files to run tests on AWS """ actions_awsfulltest: Optional[bool] = None """ Lint all required files to run full tests on AWS """ - readme: Optional[bool] = None - """ Lint the README.md file """ pipeline_todos: Optional[bool] = None """ Lint for TODOs statements""" plugin_includes: Optional[bool] = None @@ -1178,6 +1178,8 @@ def __getitem__(self, item: str) -> Any: return getattr(self, item) def get(self, item: str, default: Any = None) -> Any: + if getattr(self, item, default) is None: + return default return getattr(self, item, default) def __setitem__(self, item: str, value: Any) -> None: From 61bb733943e5c1216574366da0e6dd89e83dc2c9 Mon Sep 17 00:00:00 2001 From: mashehu Date: Tue, 22 Oct 2024 15:46:09 +0200 Subject: [PATCH 15/21] drop None values when checking for test names --- nf_core/pipelines/lint/__init__.py | 2 +- tests/pipelines/lint/test_files_exist.py | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/nf_core/pipelines/lint/__init__.py b/nf_core/pipelines/lint/__init__.py index 82361565f..f24374384 100644 --- a/nf_core/pipelines/lint/__init__.py +++ b/nf_core/pipelines/lint/__init__.py @@ -178,7 +178,7 @@ def _load_lint_config(self) -> bool: # Check if we have any keys that don't match lint test names if self.lint_config is not None: for k, v in self.lint_config: - if k != "nfcore_components" and k not in self.lint_tests: + if v is not None and k != "nfcore_components" and k not in self.lint_tests: # nfcore_components is an exception to allow custom pipelines without nf-core components log.warning(f"Found unrecognised test name '{k}' in pipeline lint config") is_correct = False diff --git a/tests/pipelines/lint/test_files_exist.py b/tests/pipelines/lint/test_files_exist.py index eb1ba9a17..ebc529247 100644 --- a/tests/pipelines/lint/test_files_exist.py +++ b/tests/pipelines/lint/test_files_exist.py @@ -37,8 +37,7 @@ def test_files_exist_missing_main(self): def test_files_exist_deprecated_file(self): """Check whether deprecated file issues warning""" - nf = Path(self.new_pipeline, "parameters.settings.json") - nf.touch() + Path(self.new_pipeline, "parameters.settings.json").touch() assert self.lint_obj._load() From e2ac2b57dc152f46a364dbdac9dce405064c1320 Mon Sep 17 00:00:00 2001 From: mashehu Date: Tue, 22 Oct 2024 16:33:19 +0200 Subject: [PATCH 16/21] fix test_lint tests --- tests/pipelines/test_lint.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/pipelines/test_lint.py b/tests/pipelines/test_lint.py index 9ca29d249..ca7353d50 100644 --- a/tests/pipelines/test_lint.py +++ b/tests/pipelines/test_lint.py @@ -48,7 +48,8 @@ def test_init_pipeline_lint(self): def test_load_lint_config_not_found(self): """Try to load a linting config file that doesn't exist""" assert self.lint_obj._load_lint_config() - assert self.lint_obj.lint_config == {} + assert self.lint_obj.lint_config is not None + assert self.lint_obj.lint_config.model_dump(exclude_none=True) == {} def test_load_lint_config_ignore_all_tests(self): """Try to load a linting config file that ignores all tests""" @@ -64,7 +65,8 @@ def test_load_lint_config_ignore_all_tests(self): # Load the new lint config file and check lint_obj._load_lint_config() - assert sorted(list(lint_obj.lint_config.keys())) == sorted(lint_obj.lint_tests) + assert lint_obj.lint_config is not None + assert sorted(list(lint_obj.lint_config.model_dump(exclude_none=True))) == sorted(lint_obj.lint_tests) # Try running linting and make sure that all tests are ignored lint_obj._lint_pipeline() From 86b926b5bfcaf556daf5fadd4f033e7c6faba52d Mon Sep 17 00:00:00 2001 From: mashehu Date: Mon, 2 Dec 2024 15:51:42 +0100 Subject: [PATCH 17/21] test also the main sync function itsel --- tests/pipelines/test_sync.py | 52 ++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/tests/pipelines/test_sync.py b/tests/pipelines/test_sync.py index ffbe75510..5bd4e55aa 100644 --- a/tests/pipelines/test_sync.py +++ b/tests/pipelines/test_sync.py @@ -56,6 +56,8 @@ def mocked_requests_get(url) -> MockResponse: for branch_no in range(3, 7) ] return MockResponse(response_data, 200, url) + if url == "https://nf-co.re/pipelines.json": + return MockResponse({"remote_workflows": [{"name": "testpipeline", "topics": ["test", "pipeline"]}]}, 200, url) return MockResponse([{"html_url": url}], 404, url) @@ -398,3 +400,53 @@ def test_reset_target_dir_fake_branch(self): with pytest.raises(nf_core.pipelines.sync.SyncExceptionError) as exc_info: psync.reset_target_dir() assert exc_info.value.args[0].startswith("Could not reset to original branch `fake_branch`") + + def test_sync_success(self): + """Test successful pipeline sync with PR creation""" + # Set up GitHub auth token for PR creation + os.environ["GITHUB_AUTH_TOKEN"] = "dummy_token" + + with mock.patch("requests.get", side_effect=mocked_requests_get), mock.patch( + "requests.post", side_effect=mocked_requests_post + ) as mock_post: + psync = nf_core.pipelines.sync.PipelineSync( + self.pipeline_dir, make_pr=True, gh_username="no_existing_pr", gh_repo="response" + ) + + # Run sync + psync.sync() + + # Verify that changes were made and PR was created + self.assertTrue(psync.made_changes) + mock_post.assert_called_once() + self.assertEqual(mock_post.call_args[0][0], "https://api.github.com/repos/no_existing_pr/response/pulls") + + def test_sync_no_changes(self): + """Test pipeline sync when no changes are needed""" + with mock.patch("requests.get", side_effect=mocked_requests_get), mock.patch( + "requests.post", side_effect=mocked_requests_post + ) as mock_post: + psync = nf_core.pipelines.sync.PipelineSync(self.pipeline_dir) + + # Mock that no changes were made + psync.made_changes = False + + # Run sync + psync.sync() + + # Verify no PR was created + mock_post.assert_not_called() + + def test_sync_no_github_token(self): + """Test sync fails appropriately when GitHub token is missing""" + # Ensure GitHub token is not set + if "GITHUB_AUTH_TOKEN" in os.environ: + del os.environ["GITHUB_AUTH_TOKEN"] + + psync = nf_core.pipelines.sync.PipelineSync(self.pipeline_dir, make_pr=True) + psync.made_changes = True # Force changes to trigger PR attempt + + # Run sync and check for appropriate error + with self.assertRaises(nf_core.pipelines.sync.PullRequestExceptionError) as exc_info: + psync.sync() + self.assertIn("GITHUB_AUTH_TOKEN not set!", str(exc_info.exception)) From ee86c151a8ff9b7e0d398184badd65621074d088 Mon Sep 17 00:00:00 2001 From: mashehu Date: Mon, 2 Dec 2024 15:54:07 +0100 Subject: [PATCH 18/21] combine json parsing code --- nf_core/pipelines/sync.py | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/nf_core/pipelines/sync.py b/nf_core/pipelines/sync.py index 896adda94..6f617295e 100644 --- a/nf_core/pipelines/sync.py +++ b/nf_core/pipelines/sync.py @@ -6,7 +6,7 @@ import re import shutil from pathlib import Path -from typing import Dict, Optional, Union +from typing import Any, Dict, Optional, Union import git import questionary @@ -416,12 +416,8 @@ def close_open_template_merge_prs(self): list_prs_url = f"https://api.github.com/repos/{self.gh_repo}/pulls" with self.gh_api.cache_disabled(): list_prs_request = self.gh_api.get(list_prs_url) - try: - list_prs_json = json.loads(list_prs_request.content) - list_prs_pp = json.dumps(list_prs_json, indent=4) - except Exception: - list_prs_json = list_prs_request.content - list_prs_pp = list_prs_request.content + + list_prs_json, list_prs_pp = self._parse_json_response(list_prs_request) log.debug(f"GitHub API listing existing PRs:\n{list_prs_url}\n{list_prs_pp}") if list_prs_request.status_code != 200: @@ -462,12 +458,8 @@ def close_open_pr(self, pr) -> bool: # Update the PR status to be closed with self.gh_api.cache_disabled(): pr_request = self.gh_api.patch(url=pr["url"], data=json.dumps({"state": "closed"})) - try: - pr_request_json = json.loads(pr_request.content) - pr_request_pp = json.dumps(pr_request_json, indent=4) - except Exception: - pr_request_json = pr_request.content - pr_request_pp = pr_request.content + + pr_request_json, pr_request_pp = self._parse_json_response(pr_request) # PR update worked if pr_request.status_code == 200: @@ -481,6 +473,22 @@ def close_open_pr(self, pr) -> bool: log.warning(f"Could not close PR ('{pr_request.status_code}'):\n{pr['url']}\n{pr_request_pp}") return False + @staticmethod + def _parse_json_response(response) -> tuple[Any, str]: + """Helper method to parse JSON response and create pretty-printed string. + + Args: + response: requests.Response object + + Returns: + Tuple of (parsed_json, pretty_printed_str) + """ + try: + json_data = json.loads(response.content) + return json_data, json.dumps(json_data, indent=4) + except Exception: + return response.content, str(response.content) + def reset_target_dir(self): """ Reset the target pipeline directory. Check out the original branch. From 10e691bd4dd0c49b07e094d172905ee48e1b7a79 Mon Sep 17 00:00:00 2001 From: mashehu Date: Mon, 2 Dec 2024 17:45:30 +0100 Subject: [PATCH 19/21] remove broken test --- nf_core/pipelines/sync.py | 2 +- tests/pipelines/test_sync.py | 20 -------------------- 2 files changed, 1 insertion(+), 21 deletions(-) diff --git a/nf_core/pipelines/sync.py b/nf_core/pipelines/sync.py index 6f617295e..8ea561bd3 100644 --- a/nf_core/pipelines/sync.py +++ b/nf_core/pipelines/sync.py @@ -120,7 +120,7 @@ def __init__( requests.auth.HTTPBasicAuth(self.gh_username, os.environ["GITHUB_AUTH_TOKEN"]) ) - def sync(self): + def sync(self) -> None: """Find workflow attributes, create a new template pipeline on TEMPLATE""" # Clear requests_cache so that we don't get stale API responses diff --git a/tests/pipelines/test_sync.py b/tests/pipelines/test_sync.py index 5bd4e55aa..8bf8a3c4e 100644 --- a/tests/pipelines/test_sync.py +++ b/tests/pipelines/test_sync.py @@ -401,26 +401,6 @@ def test_reset_target_dir_fake_branch(self): psync.reset_target_dir() assert exc_info.value.args[0].startswith("Could not reset to original branch `fake_branch`") - def test_sync_success(self): - """Test successful pipeline sync with PR creation""" - # Set up GitHub auth token for PR creation - os.environ["GITHUB_AUTH_TOKEN"] = "dummy_token" - - with mock.patch("requests.get", side_effect=mocked_requests_get), mock.patch( - "requests.post", side_effect=mocked_requests_post - ) as mock_post: - psync = nf_core.pipelines.sync.PipelineSync( - self.pipeline_dir, make_pr=True, gh_username="no_existing_pr", gh_repo="response" - ) - - # Run sync - psync.sync() - - # Verify that changes were made and PR was created - self.assertTrue(psync.made_changes) - mock_post.assert_called_once() - self.assertEqual(mock_post.call_args[0][0], "https://api.github.com/repos/no_existing_pr/response/pulls") - def test_sync_no_changes(self): """Test pipeline sync when no changes are needed""" with mock.patch("requests.get", side_effect=mocked_requests_get), mock.patch( From 11f7f426ce937c53f17ff210a952413d8ad7b408 Mon Sep 17 00:00:00 2001 From: mashehu Date: Mon, 2 Dec 2024 21:16:57 +0100 Subject: [PATCH 20/21] fix type error --- nf_core/pipelines/sync.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nf_core/pipelines/sync.py b/nf_core/pipelines/sync.py index 8ea561bd3..781b4f5f0 100644 --- a/nf_core/pipelines/sync.py +++ b/nf_core/pipelines/sync.py @@ -6,7 +6,7 @@ import re import shutil from pathlib import Path -from typing import Any, Dict, Optional, Union +from typing import Any, Dict, Optional, Tuple, Union import git import questionary @@ -474,7 +474,7 @@ def close_open_pr(self, pr) -> bool: return False @staticmethod - def _parse_json_response(response) -> tuple[Any, str]: + def _parse_json_response(response) -> Tuple[Any, str]: """Helper method to parse JSON response and create pretty-printed string. Args: From eed0598c7baf5f20e87254cd8a95bc29b0836fc8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matthias=20H=C3=B6rtenhuber?= Date: Tue, 3 Dec 2024 12:02:50 +0100 Subject: [PATCH 21/21] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Júlia Mir Pedrol --- nf_core/pipelines/lint/__init__.py | 2 +- nf_core/pipelines/lint/readme.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/nf_core/pipelines/lint/__init__.py b/nf_core/pipelines/lint/__init__.py index f24374384..154e38aea 100644 --- a/nf_core/pipelines/lint/__init__.py +++ b/nf_core/pipelines/lint/__init__.py @@ -593,7 +593,7 @@ def run_linting( lint_obj._load_lint_config() lint_obj.load_pipeline_config() - if lint_obj.lint_config and lint_obj.lint_config["nfcore_components"] is False: + if lint_obj.lint_config and not lint_obj.lint_config["nfcore_components"]: module_lint_obj = None subworkflow_lint_obj = None else: diff --git a/nf_core/pipelines/lint/readme.py b/nf_core/pipelines/lint/readme.py index 5a10fbfce..75b05f16e 100644 --- a/nf_core/pipelines/lint/readme.py +++ b/nf_core/pipelines/lint/readme.py @@ -35,8 +35,8 @@ def readme(self): lint: readme: - nextflow_badge - zenodo_release + - nextflow_badge + - zenodo_release """ passed = []