diff --git a/.github/workflows/fix-linting.yml b/.github/workflows/fix-linting.yml index 595fff66ae..95a03c70fe 100644 --- a/.github/workflows/fix-linting.yml +++ b/.github/workflows/fix-linting.yml @@ -10,7 +10,7 @@ jobs: contains(github.event.comment.html_url, '/pull/') && contains(github.event.comment.body, '@nf-core-bot fix linting') && github.repository == 'nf-core/tools' - runs-on: self-hosted + runs-on: ubuntu-latest steps: # Use the @nf-core-bot token to check out so we can push later - uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4 diff --git a/CHANGELOG.md b/CHANGELOG.md index 5014bc9e16..23d42cab9a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ ### Linting - Handle default values of type number from nextflow schema ([#2703](https://github.com/nf-core/tools/pull/2703)) +- fix ignoring files_unchanged ([#2707](https://github.com/nf-core/tools/pull/2707)) ### Modules diff --git a/nf_core/lint/__init__.py b/nf_core/lint/__init__.py index 4f7657aec5..d39ce5d7e9 100644 --- a/nf_core/lint/__init__.py +++ b/nf_core/lint/__init__.py @@ -8,6 +8,8 @@ import json import logging import os +from pathlib import Path +from typing import List, Union import git import rich @@ -621,7 +623,7 @@ def _save_json_results(self, json_fn): with open(json_fn, "w") as fh: json.dump(results, fh, indent=4) - def _wrap_quotes(self, files): + def _wrap_quotes(self, files: Union[List[str], List[Path], Path]) -> str: """Helper function to take a list of filenames and format with markdown. Args: @@ -636,5 +638,5 @@ def _wrap_quotes(self, files): """ if not isinstance(files, list): files = [files] - bfiles = [f"`{f}`" for f in files] + bfiles = [f"`{str(f)}`" for f in files] return " or ".join(bfiles) diff --git a/nf_core/lint/files_exist.py b/nf_core/lint/files_exist.py index 78a744a4d7..1bd6dba74c 100644 --- a/nf_core/lint/files_exist.py +++ b/nf_core/lint/files_exist.py @@ -1,11 +1,11 @@ import logging from pathlib import Path -from typing import Union +from typing import Dict, List, Tuple, Union log = logging.getLogger(__name__) -def files_exist(self): +def files_exist(self) -> Dict[str, Union[List[str], bool]]: """Checks a given pipeline directory for required files. Iterates through the pipeline's directory content and checks that specified @@ -129,19 +129,19 @@ def files_exist(self): short_name = self.nf_config["manifest.name"].strip("\"'").split("/") files_fail = [ - [".gitattributes"], - [".gitignore"], - [".nf-core.yml"], - [".editorconfig"], - [".prettierignore"], - [".prettierrc.yml"], - ["CHANGELOG.md"], - ["CITATIONS.md"], - ["CODE_OF_CONDUCT.md"], - ["LICENSE", "LICENSE.md", "LICENCE", "LICENCE.md"], # NB: British / American spelling - ["nextflow_schema.json"], - ["nextflow.config"], - ["README.md"], + [Path(".gitattributes")], + [Path(".gitignore")], + [Path(".nf-core.yml")], + [Path(".editorconfig")], + [Path(".prettierignore")], + [Path(".prettierrc.yml")], + [Path("CHANGELOG.md")], + [Path("CITATIONS.md")], + [Path("CODE_OF_CONDUCT.md")], + [Path("LICENSE"), Path("LICENSE.md"), Path("LICENCE"), Path("LICENCE.md")], # NB: British / American spelling + [Path("nextflow_schema.json")], + [Path("nextflow.config")], + [Path("README.md")], [Path(".github", ".dockstore.yml")], [Path(".github", "CONTRIBUTING.md")], [Path(".github", "ISSUE_TEMPLATE", "bug_report.yml")], @@ -171,41 +171,42 @@ def files_exist(self): ] files_warn = [ - ["main.nf"], + [Path("main.nf")], [Path("assets", "multiqc_config.yml")], [Path("conf", "base.config")], [Path("conf", "igenomes.config")], [Path(".github", "workflows", "awstest.yml")], [Path(".github", "workflows", "awsfulltest.yml")], [Path("lib", f"Workflow{short_name[0].upper()}{short_name[1:]}.groovy")], - ["modules.json"], - ["pyproject.toml"], + [Path("modules.json")], + [Path("pyproject.toml")], ] # List of strings. Fails / warns if any of the strings exist. files_fail_ifexists = [ - "Singularity", - "parameters.settings.json", - "pipeline_template.yml", # saving information in .nf-core.yml - ".nf-core.yaml", # yml not yaml + Path("Singularity"), + Path("parameters.settings.json"), + Path("pipeline_template.yml"), # saving information in .nf-core.yml + Path(".nf-core.yaml"), # yml not yaml Path("bin", "markdown_to_html.r"), Path("conf", "aws.config"), Path(".github", "workflows", "push_dockerhub.yml"), Path(".github", "ISSUE_TEMPLATE", "bug_report.md"), Path(".github", "ISSUE_TEMPLATE", "feature_request.md"), Path("docs", "images", f"nf-core-{short_name}_logo.png"), - ".markdownlint.yml", - ".yamllint.yml", + Path(".markdownlint.yml"), + Path(".yamllint.yml"), Path("lib", "Checks.groovy"), Path("lib", "Completion.groovy"), Path("lib", "Workflow.groovy"), ] - files_warn_ifexists = [".travis.yml"] - files_fail_ifinconfig = [[Path("lib", "nfcore_external_java_deps.jar"), "nf-validation"]] + files_warn_ifexists = [Path(".travis.yml")] + files_fail_ifinconfig: List[Tuple[Path, Dict[str, str]]] = [ + (Path("lib", "nfcore_external_java_deps.jar"), {"plugins": "nf-validation"}), + ] # Remove files that should be ignored according to the linting config ignore_files = self.lint_config.get("files_exist", []) - log.info(f"Files to ignore: {ignore_files}") def pf(file_path: Union[str, Path]) -> Path: return Path(self.wf_path, file_path) @@ -242,32 +243,32 @@ def pf(file_path: Union[str, Path]) -> Path: else: passed.append(f"File not found check: {self._wrap_quotes(file)}") # Files that cause an error if they exists together with a certain entry in nextflow.config - for file in files_fail_ifinconfig: - if str(file[0]) in ignore_files: + for file_cond in files_fail_ifinconfig: + if str(file_cond[0]) in ignore_files: continue - nextflow_config = pf("nextflow.config") in_config = False - with open(nextflow_config) as f: - if file[1] in f.read(): - in_config = True - if pf(file[0]).is_file() and in_config: - failed.append(f"File must be removed: {self._wrap_quotes(file[0])}") - elif pf(file[0]).is_file() and not in_config: - passed.append(f"File found check: {self._wrap_quotes(file[0])}") - elif not pf(file[0]).is_file() and not in_config: - failed.append(f"File not found check: {self._wrap_quotes(file[0])}") - elif not pf(file[0]).is_file() and in_config: - passed.append(f"File not found check: {self._wrap_quotes(file[0])}") + config_key, config_value = list(file_cond[1].items())[0] + if config_key in self.nf_config and config_value in self.nf_config[config_key]: + log.debug(f"Found {config_key} in nextflow.config with value {config_value}") + in_config = True + if pf(file_cond[0]).is_file() and in_config: + failed.append(f"File must be removed: {self._wrap_quotes(file_cond[0])}") + elif pf(file_cond[0]).is_file() and not in_config: + passed.append(f"File found check: {self._wrap_quotes(file_cond[0])}") + elif not pf(file_cond[0]).is_file() and not in_config: + failed.append(f"File not found check: {self._wrap_quotes(file_cond[0])}") + elif not pf(file_cond[0]).is_file() and in_config: + passed.append(f"File not found check: {self._wrap_quotes(file_cond[0])}") # Files that cause a warning if they exist for file in files_warn_ifexists: - if file in ignore_files: + if str(file) in ignore_files: continue if pf(file).is_file(): warned.append(f"File should be removed: {self._wrap_quotes(file)}") else: passed.append(f"File not found check: {self._wrap_quotes(file)}") - # Files that are ignoed + # Files that are ignored for file in ignore_files: ignored.append(f"File is ignored: {self._wrap_quotes(file)}") diff --git a/nf_core/lint/files_unchanged.py b/nf_core/lint/files_unchanged.py index 176b0e9e65..399830faae 100644 --- a/nf_core/lint/files_unchanged.py +++ b/nf_core/lint/files_unchanged.py @@ -3,7 +3,7 @@ import shutil import tempfile from pathlib import Path -from typing import Union +from typing import Dict, List, Union import yaml @@ -12,7 +12,7 @@ log = logging.getLogger(__name__) -def files_unchanged(self): +def files_unchanged(self) -> Dict[str, Union[List[str], bool]]: """Checks that certain pipeline files are not modified from template output. Iterates through the pipeline's directory content and compares specified files @@ -49,9 +49,6 @@ def files_unchanged(self): .prettierignore pyproject.toml - Files that need to be there or not based on a entry in nextflow config:: - - lib/nfcore_external_java_deps.jar # if config doesn't mention nf-validation .. tip:: You can configure the ``nf-core lint`` tests to ignore any of these checks by setting the ``files_unchanged`` key as follows in your ``.nf-core.yml`` config file. For example: @@ -64,11 +61,11 @@ def files_unchanged(self): """ - passed = [] - failed = [] - ignored = [] - fixed = [] - could_fix = False + passed: List[str] = [] + failed: List[str] = [] + ignored: List[str] = [] + fixed: List[str] = [] + could_fix: bool = False # Check that we have the minimum required config required_pipeline_config = {"manifest.name", "manifest.description", "manifest.author"} @@ -87,10 +84,10 @@ def files_unchanged(self): # NB: Should all be files, not directories # List of lists. Passes if any of the files in the sublist are found. files_exact = [ - [".gitattributes"], - [".prettierrc.yml"], - ["CODE_OF_CONDUCT.md"], - ["LICENSE", "LICENSE.md", "LICENCE", "LICENCE.md"], # NB: British / American spelling + [Path(".gitattributes")], + [Path(".prettierrc.yml")], + [Path("CODE_OF_CONDUCT.md")], + [Path("LICENSE"), Path("LICENSE.md"), Path("LICENCE"), Path("LICENCE.md")], # NB: British / American spelling [Path(".github", ".dockstore.yml")], [Path(".github", "CONTRIBUTING.md")], [Path(".github", "ISSUE_TEMPLATE", "bug_report.yml")], @@ -110,10 +107,7 @@ def files_unchanged(self): [Path("lib", "NfcoreTemplate.groovy")], ] files_partial = [ - [".gitignore", ".prettierignore", "pyproject.toml"], - ] - files_conditional = [ - [Path("lib", "nfcore_external_java_deps.jar"), {"plugins": "nf_validation"}], + [Path(".gitignore"), Path(".prettierignore"), Path("pyproject.toml")], ] # Only show error messages from pipeline creation @@ -149,11 +143,12 @@ def _tf(file_path: Union[str, Path]) -> Path: """Helper function - get file path for template file""" return Path(test_pipeline_dir, file_path) + ignore_files = self.lint_config.get("files_unchanged", []) + # Files that must be completely unchanged from template for files in files_exact: # Ignore if file specified in linting config - ignore_files = self.lint_config.get("files_unchanged", []) - if any([f in ignore_files for f in files]): + if any([str(f) in ignore_files for f in files]): ignored.append(f"File ignored due to lint config: {self._wrap_quotes(files)}") # Ignore if we can't find the file @@ -181,7 +176,6 @@ def _tf(file_path: Union[str, Path]) -> Path: # Files that can be added to, but that must contain the template contents for files in files_partial: # Ignore if file specified in linting config - ignore_files = self.lint_config.get("files_unchanged", []) if any([f in ignore_files for f in files]): ignored.append(f"File ignored due to lint config: {self._wrap_quotes(files)}") @@ -214,39 +208,6 @@ def _tf(file_path: Union[str, Path]) -> Path: except FileNotFoundError: pass - # Files that should be there only if an entry in nextflow config is not set - for files in files_conditional: - # Ignore if file specified in linting config - ignore_files = self.lint_config.get("files_unchanged", []) - if files[0] in ignore_files: - ignored.append(f"File ignored due to lint config: {self._wrap_quotes(files)}") - - # Ignore if we can't find the file - elif _pf(files[0]).is_file(): - ignored.append(f"File does not exist: {self._wrap_quotes(files[0])}") - - # Check that the file has an identical match - else: - config_key, config_value = list(files[1].items())[0] - if config_key in self.nf_config and self.nf_config[config_key] == config_value: - # Ignore if the config key is set to the expected value - ignored.append(f"File ignored due to config: {self._wrap_quotes(files)}") - else: - try: - if filecmp.cmp(_pf(files[0]), _tf(files[0]), shallow=True): - passed.append(f"`{files[0]}` matches the template") - else: - if "files_unchanged" in self.fix: - # Try to fix the problem by overwriting the pipeline file - shutil.copy(_tf(files[0]), _pf(files[0])) - passed.append(f"`{files[0]}` matches the template") - fixed.append(f"`{files[0]}` overwritten with template file") - else: - failed.append(f"`{files[0]}` does not match the template") - could_fix = True - except FileNotFoundError: - pass - # cleaning up temporary dir shutil.rmtree(tmp_dir) diff --git a/tests/lint/files_exist.py b/tests/lint/files_exist.py index 5ba26d77a0..d3a6a25e82 100644 --- a/tests/lint/files_exist.py +++ b/tests/lint/files_exist.py @@ -54,3 +54,24 @@ def test_files_exist_pass(self): results = lint_obj.files_exist() assert results["failed"] == [] + + +def test_files_exist_pass_conditional(self): + new_pipeline = self._make_pipeline_copy() + lint_obj = nf_core.lint.PipelineLint(new_pipeline) + lint_obj._load() + lint_obj.nf_config["plugins"] = [] + Path(new_pipeline, "lib/nfcore_external_java_deps.jar").touch() + results = lint_obj.files_exist() + assert results["failed"] == [] + assert results["ignored"] == [] + + +def test_files_exist_fail_conditional(self): + new_pipeline = self._make_pipeline_copy() + lint_obj = nf_core.lint.PipelineLint(new_pipeline) + lint_obj._load() + Path(new_pipeline, "lib/nfcore_external_java_deps.jar").touch() + results = lint_obj.files_exist() + assert results["failed"] == ["File must be removed: `lib/nfcore_external_java_deps.jar`"] + assert results["ignored"] == [] diff --git a/tests/lint/files_unchanged.py b/tests/lint/files_unchanged.py index 84aec50c26..601f09b9df 100644 --- a/tests/lint/files_unchanged.py +++ b/tests/lint/files_unchanged.py @@ -1,4 +1,4 @@ -import os +from pathlib import Path import nf_core.lint @@ -13,14 +13,14 @@ def test_files_unchanged_pass(self): def test_files_unchanged_fail(self): - failing_file = os.path.join(".github", "CONTRIBUTING.md") + failing_file = Path(".github", "CONTRIBUTING.md") new_pipeline = self._make_pipeline_copy() - with open(os.path.join(new_pipeline, failing_file), "a") as fh: + with open(Path(new_pipeline, failing_file), "a") as fh: fh.write("THIS SHOULD NOT BE HERE") lint_obj = nf_core.lint.PipelineLint(new_pipeline) lint_obj._load() results = lint_obj.files_unchanged() assert len(results["failed"]) > 0 - assert failing_file in results["failed"][0] + assert str(failing_file) in results["failed"][0] assert results["could_fix"] diff --git a/tests/test_lint.py b/tests/test_lint.py index c8d7135654..2f989aeffb 100644 --- a/tests/test_lint.py +++ b/tests/test_lint.py @@ -200,9 +200,11 @@ def test_sphinx_md_files(self): ) from .lint.files_exist import ( # type: ignore[misc] test_files_exist_depreciated_file, + test_files_exist_fail_conditional, test_files_exist_missing_config, test_files_exist_missing_main, test_files_exist_pass, + test_files_exist_pass_conditional, ) from .lint.files_unchanged import ( # type: ignore[misc] test_files_unchanged_fail,