Merge pull request #2707 from mirpedrol/files-unchanged

Linting: fix ignoring files_unchanged

.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

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
 

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)

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)}")
 

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)
 

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"] == []

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"]

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,