From fbb1759a036444f13b9e96e77812a82d5a50e0ab Mon Sep 17 00:00:00 2001 From: fabianegli Date: Sat, 29 Oct 2022 12:25:51 +0200 Subject: [PATCH 01/12] use f-string instead of format --- nf_core/lint_utils.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/nf_core/lint_utils.py b/nf_core/lint_utils.py index 19d253c53c..b412772259 100644 --- a/nf_core/lint_utils.py +++ b/nf_core/lint_utils.py @@ -37,16 +37,18 @@ def print_joint_summary(lint_obj, module_lint_obj): def print_fixes(lint_obj, module_lint_obj): """Prints available and applied fixes""" - if len(lint_obj.could_fix): - fix_cmd = "nf-core lint {} --fix {}".format( - "" if lint_obj.wf_path == "." else f"--dir {lint_obj.wf_path}", " --fix ".join(lint_obj.could_fix) - ) + fixe_flags = " ".join([f"--fix {file}" for file in lint_obj.could_fix]) + + if fixe_flags: + lint_dir = "" if lint_obj.wf_path == "." else f"--dir {lint_obj.wf_path}" console.print( - f"\nTip: Some of these linting errors can automatically be resolved with the following command:\n\n[blue] {fix_cmd}\n" + "\nTip: Some of these linting errors can automatically be resolved with the following command:" + f"\n\n[blue] nf-core lint {lint_dir} {fixe_flags}\n" ) if len(lint_obj.fix): console.print( - "Automatic fixes applied. Please check with 'git diff' and revert any changes you do not want with 'git checkout '." + "Automatic fixes applied. Please check with 'git diff' and revert " + "any changes you do not want with 'git checkout '." ) From b1882d8a4a13abfaebb4fdfcef57b7edd1f3151f Mon Sep 17 00:00:00 2001 From: fabianegli Date: Sat, 29 Oct 2022 12:28:16 +0200 Subject: [PATCH 02/12] use pre-commit installed Prettier if not already installed --- nf_core/lint_utils.py | 50 +++++++++++++++++++++++++++++++++++-------- requirements-dev.txt | 1 - requirements.txt | 1 + 3 files changed, 42 insertions(+), 10 deletions(-) diff --git a/nf_core/lint_utils.py b/nf_core/lint_utils.py index b412772259..61b472fcf6 100644 --- a/nf_core/lint_utils.py +++ b/nf_core/lint_utils.py @@ -1,4 +1,5 @@ import logging +import shutil import subprocess import rich @@ -53,7 +54,7 @@ def print_fixes(lint_obj, module_lint_obj): def run_prettier_on_file(file): - """Runs Prettier on a file if Prettier is installed. + """Run Prettier on a file. Args: file (Path | str): A file identifier as a string or pathlib.Path. @@ -62,12 +63,43 @@ def run_prettier_on_file(file): If Prettier is not installed, a warning is logged. """ - try: - subprocess.run( - ["prettier", "--write", file], - stdout=subprocess.DEVNULL, - stderr=subprocess.DEVNULL, - check=False, + _prettier_installed = not shutil.which("prettier") is None + _pre_commit_installed = not shutil.which("pre-commit") is None + + if _prettier_installed: + _run_prettier_on_file(file) + elif _pre_commit_installed: + _run_pre_commit_prettier_on_file(file) + else: + log.warning( + "Neither Prettier nor the prettier pre-commit hook are available. At least one of them is required." ) - except FileNotFoundError: - log.warning("Prettier is not installed. Please install it and run it on the pipeline to fix linting issues.") + + +def _run_prettier_on_file(file): + """Run natively installed Prettier on a file.""" + + subprocess.run( + ["prettier", "--write", file], + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + check=False, + ) + + +def _run_pre_commit_prettier_on_file(file): + """Runs pre-commit hook prettier on a file if pre-commit is installed. + + Args: + file (Path | str): A file identifier as a string or pathlib.Path. + + Warns: + If Prettier is not installed, a warning is logged. + """ + + subprocess.run( + ["pre-commit", "run", "prettier", file], + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + check=False, + ) diff --git a/requirements-dev.txt b/requirements-dev.txt index 8fc8d1c7de..3809ae1c1d 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -1,7 +1,6 @@ black isort myst_parser -pre-commit pytest-cov pytest-datafiles Sphinx diff --git a/requirements.txt b/requirements.txt index 0a4a5fb7e7..fcc5130f8b 100644 --- a/requirements.txt +++ b/requirements.txt @@ -5,6 +5,7 @@ jinja2 jsonschema>=3.0 markdown>=3.3 packaging +pre-commit prompt_toolkit>=3.0.3 pytest>=7.0.0 pytest-workflow>=1.6.0 From a9a9be9031ecd6149dd2976aeebc90077505c3fd Mon Sep 17 00:00:00 2001 From: fabianegli Date: Sat, 29 Oct 2022 12:57:22 +0200 Subject: [PATCH 03/12] ship and use the nf-core pre-commit config for linting --- MANIFEST.in | 1 + nf_core/lint_utils.py | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/MANIFEST.in b/MANIFEST.in index 384a7b5441..7363b92974 100644 --- a/MANIFEST.in +++ b/MANIFEST.in @@ -4,3 +4,4 @@ graft nf_core/module-template graft nf_core/pipeline-template graft nf_core/subworkflow-template include requirements.txt +include .pre-commit-config.yaml diff --git a/nf_core/lint_utils.py b/nf_core/lint_utils.py index 61b472fcf6..d50bb51e41 100644 --- a/nf_core/lint_utils.py +++ b/nf_core/lint_utils.py @@ -1,6 +1,7 @@ import logging import shutil import subprocess +from pathlib import Path import rich from rich.console import Console @@ -97,8 +98,10 @@ def _run_pre_commit_prettier_on_file(file): If Prettier is not installed, a warning is logged. """ + nf_core_pre_commit_config = Path(nf_core.__file__).parent.parent / ".pre-commit-config.yaml" + subprocess.run( - ["pre-commit", "run", "prettier", file], + ["pre-commit", "run", f"--config {nf_core_pre_commit_config}", "prettier", f"--files {file}"], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, check=False, From 5860858ea48591a93b3318d4f105ab2f31c66f3e Mon Sep 17 00:00:00 2001 From: fabianegli Date: Sat, 29 Oct 2022 14:29:04 +0200 Subject: [PATCH 04/12] fix typo in variable name --- nf_core/lint_utils.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nf_core/lint_utils.py b/nf_core/lint_utils.py index d50bb51e41..bf9e9519ae 100644 --- a/nf_core/lint_utils.py +++ b/nf_core/lint_utils.py @@ -39,9 +39,9 @@ def print_joint_summary(lint_obj, module_lint_obj): def print_fixes(lint_obj, module_lint_obj): """Prints available and applied fixes""" - fixe_flags = " ".join([f"--fix {file}" for file in lint_obj.could_fix]) + fix_flags = " ".join([f"--fix {file}" for file in lint_obj.could_fix]) - if fixe_flags: + if fix_flags: lint_dir = "" if lint_obj.wf_path == "." else f"--dir {lint_obj.wf_path}" console.print( "\nTip: Some of these linting errors can automatically be resolved with the following command:" From 1e46a5d023392f852eb60ba882d6cb3ec15a430b Mon Sep 17 00:00:00 2001 From: fabianegli Date: Sat, 5 Nov 2022 21:37:47 +0100 Subject: [PATCH 05/12] simplify conditions and utilize lazy eval of elif --- nf_core/lint_utils.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/nf_core/lint_utils.py b/nf_core/lint_utils.py index bf9e9519ae..feb8dfac1c 100644 --- a/nf_core/lint_utils.py +++ b/nf_core/lint_utils.py @@ -64,12 +64,9 @@ def run_prettier_on_file(file): If Prettier is not installed, a warning is logged. """ - _prettier_installed = not shutil.which("prettier") is None - _pre_commit_installed = not shutil.which("pre-commit") is None - - if _prettier_installed: + if shutil.which("prettier"): _run_prettier_on_file(file) - elif _pre_commit_installed: + elif shutil.which("pre-commit"): _run_pre_commit_prettier_on_file(file) else: log.warning( From d648284ad9c908bdfe4d2a2a84505aa581f40db9 Mon Sep 17 00:00:00 2001 From: fabianegli Date: Sun, 6 Nov 2022 10:46:04 +0100 Subject: [PATCH 06/12] fix typo in variable name --- nf_core/lint_utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nf_core/lint_utils.py b/nf_core/lint_utils.py index feb8dfac1c..07b7f8ba21 100644 --- a/nf_core/lint_utils.py +++ b/nf_core/lint_utils.py @@ -45,7 +45,7 @@ def print_fixes(lint_obj, module_lint_obj): lint_dir = "" if lint_obj.wf_path == "." else f"--dir {lint_obj.wf_path}" console.print( "\nTip: Some of these linting errors can automatically be resolved with the following command:" - f"\n\n[blue] nf-core lint {lint_dir} {fixe_flags}\n" + f"\n\n[blue] nf-core lint {lint_dir} {fix_flags}\n" ) if len(lint_obj.fix): console.print( From d143da64a5c51563cab78169e4e0977921431a7b Mon Sep 17 00:00:00 2001 From: fabianegli Date: Mon, 7 Nov 2022 08:02:32 +0100 Subject: [PATCH 07/12] remove unused argument --- nf_core/lint/__init__.py | 2 +- nf_core/lint_utils.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/nf_core/lint/__init__.py b/nf_core/lint/__init__.py index 5ea01142e5..91391794c3 100644 --- a/nf_core/lint/__init__.py +++ b/nf_core/lint/__init__.py @@ -124,7 +124,7 @@ def run_linting( lint_obj._print_results(show_passed) module_lint_obj._print_results(show_passed) nf_core.lint_utils.print_joint_summary(lint_obj, module_lint_obj) - nf_core.lint_utils.print_fixes(lint_obj, module_lint_obj) + nf_core.lint_utils.print_fixes(lint_obj) # Save results to Markdown file if md_fn is not None: diff --git a/nf_core/lint_utils.py b/nf_core/lint_utils.py index 07b7f8ba21..1d2078ad94 100644 --- a/nf_core/lint_utils.py +++ b/nf_core/lint_utils.py @@ -36,7 +36,7 @@ def print_joint_summary(lint_obj, module_lint_obj): console.print(table) -def print_fixes(lint_obj, module_lint_obj): +def print_fixes(lint_obj): """Prints available and applied fixes""" fix_flags = " ".join([f"--fix {file}" for file in lint_obj.could_fix]) From dcb8ac05b56b5e4f78912dcc380f9b727c688445 Mon Sep 17 00:00:00 2001 From: fabianegli Date: Mon, 7 Nov 2022 08:03:22 +0100 Subject: [PATCH 08/12] only compute fix flags if needed --- nf_core/lint_utils.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/nf_core/lint_utils.py b/nf_core/lint_utils.py index 1d2078ad94..7f904b756c 100644 --- a/nf_core/lint_utils.py +++ b/nf_core/lint_utils.py @@ -39,10 +39,9 @@ def print_joint_summary(lint_obj, module_lint_obj): def print_fixes(lint_obj): """Prints available and applied fixes""" - fix_flags = " ".join([f"--fix {file}" for file in lint_obj.could_fix]) - - if fix_flags: + if lint_obj.could_fix: lint_dir = "" if lint_obj.wf_path == "." else f"--dir {lint_obj.wf_path}" + fix_flags = " ".join([f"--fix {file}" for file in lint_obj.could_fix]) console.print( "\nTip: Some of these linting errors can automatically be resolved with the following command:" f"\n\n[blue] nf-core lint {lint_dir} {fix_flags}\n" From 9c95c5c33ad8f849cc171d6cd3ee7f7607964e32 Mon Sep 17 00:00:00 2001 From: fabianegli Date: Mon, 14 Nov 2022 12:37:04 +0100 Subject: [PATCH 09/12] make running prettier return better error messages --- nf_core/lint_utils.py | 39 ++++++++++++++++++++++++++------------- 1 file changed, 26 insertions(+), 13 deletions(-) diff --git a/nf_core/lint_utils.py b/nf_core/lint_utils.py index 7f904b756c..31519f9dc6 100644 --- a/nf_core/lint_utils.py +++ b/nf_core/lint_utils.py @@ -76,12 +76,19 @@ def run_prettier_on_file(file): def _run_prettier_on_file(file): """Run natively installed Prettier on a file.""" - subprocess.run( - ["prettier", "--write", file], - stdout=subprocess.DEVNULL, - stderr=subprocess.DEVNULL, - check=False, - ) + try: + subprocess.run( + ["prettier", "--write", file], + capture_output=True, + check=True, + ) + except subprocess.CalledProcessError as e: + if ": SyntaxError: " in e.stderr.decode(): + raise ValueError(f"Can't format {file} because it has a synthax error.\n{e.stderr.decode()}") from e + raise ValueError( + "There was an error running the prettier pre-commit hook.\n" + f"STDOUT: {e.stdout.decode()}\nSTDERR: {e.stderr.decode()}" + ) from e def _run_pre_commit_prettier_on_file(file): @@ -95,10 +102,16 @@ def _run_pre_commit_prettier_on_file(file): """ nf_core_pre_commit_config = Path(nf_core.__file__).parent.parent / ".pre-commit-config.yaml" - - subprocess.run( - ["pre-commit", "run", f"--config {nf_core_pre_commit_config}", "prettier", f"--files {file}"], - stdout=subprocess.DEVNULL, - stderr=subprocess.DEVNULL, - check=False, - ) + try: + subprocess.run( + ["pre-commit", "run", "--config", nf_core_pre_commit_config, "prettier", "--files", file], + capture_output=True, + check=True, + ) + except subprocess.CalledProcessError as e: + if ": SyntaxError: " in e.stdout.decode(): + raise ValueError(f"Can't format {file} because it has a synthax error.\n{e.stdout.decode()}") from e + raise ValueError( + "There was an error running the prettier pre-commit hook.\n" + f"STDOUT: {e.stdout.decode()}\nSTDERR: {e.stderr.decode()}" + ) from e From 22819d9340f998546da38ed2349d5ff77fc4af89 Mon Sep 17 00:00:00 2001 From: fabianegli Date: Mon, 14 Nov 2022 12:37:26 +0100 Subject: [PATCH 10/12] add tests for prettier invocations --- tests/test_lint_utils.py | 100 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 100 insertions(+) create mode 100644 tests/test_lint_utils.py diff --git a/tests/test_lint_utils.py b/tests/test_lint_utils.py new file mode 100644 index 0000000000..1d864d77df --- /dev/null +++ b/tests/test_lint_utils.py @@ -0,0 +1,100 @@ +import shutil +from pathlib import Path +from unittest import mock + +import git +import pytest + +import nf_core.lint_utils + +JSON_WITH_SYNTAX_ERROR = "{'a':1, 1}" +JSON_MALFORMED = "{'a':1}" +JSON_FORMATTED = '{ "a": 1 }\n' + +WHICH_PRE_COMMIT = shutil.which("pre-commit") + + +@pytest.fixture() +def temp_git_repo(tmp_path_factory): + tmp_git_dir = tmp_path_factory.mktemp("tmp_git_dir") + repo = git.Repo.init(tmp_git_dir) + return tmp_git_dir, repo + + +@pytest.fixture(name="formatted_json") +def git_dir_with_json(temp_git_repo): + tmp_git_dir, repo = temp_git_repo + file = tmp_git_dir / "formatted.json" + with open(file, "w", encoding="utf-8") as f: + f.write(JSON_FORMATTED) + repo.git.add(file) + return file + + +@pytest.fixture(name="malformed_json") +def git_dir_with_json_malformed(temp_git_repo): + tmp_git_dir, repo = temp_git_repo + file = tmp_git_dir / "malformed.json" + with open(file, "w", encoding="utf-8") as f: + f.write(JSON_MALFORMED) + repo.git.add(file) + return file + + +@pytest.fixture(name="synthax_error_json") +def git_dir_with_json_syntax_error(temp_git_repo): + tmp_git_dir, repo = temp_git_repo + file = tmp_git_dir / "synthax-error.json" + with open(file, "w", encoding="utf-8") as f: + f.write(JSON_WITH_SYNTAX_ERROR) + repo.git.add(file) + return file + + +@pytest.fixture +def ensure_prettier_is_not_found(monkeypatch): + def dont_find_prettier(x): + if x == "pre-commit": + which_x = WHICH_PRE_COMMIT + elif x == "prettier": + which_x = None + else: + raise ValueError(f"This mock is only inteded to hide prettier form tests. {x}") + return which_x + + monkeypatch.setattr("nf_core.lint_utils.shutil.which", dont_find_prettier) + + +@pytest.mark.skipif(shutil.which("prettier") is None, reason="Can't test prettier if it is not available.") +def test_run_prettier_on_formatted_file(formatted_json): + nf_core.lint_utils.run_prettier_on_file(formatted_json) + assert formatted_json.read_text() == JSON_FORMATTED + + +def test_run_pre_commit_prettier_on_formatted_file(formatted_json, ensure_prettier_is_not_found): + nf_core.lint_utils.run_prettier_on_file(formatted_json) + assert formatted_json.read_text() == JSON_FORMATTED + + +@pytest.mark.skipif(shutil.which("prettier") is None, reason="Can't test prettier if it is not available.") +def test_run_prettier_on_malformed_file(malformed_json): + nf_core.lint_utils.run_prettier_on_file(malformed_json) + assert malformed_json.read_text() == JSON_FORMATTED + + +def test_run_prettier_pre_commit_on_malformed_file(malformed_json, ensure_prettier_is_not_found): + nf_core.lint_utils.run_prettier_on_file(malformed_json) + assert malformed_json.read_text() == JSON_FORMATTED + + +@pytest.mark.skipif(shutil.which("prettier") is None, reason="Can't test prettier if it is not available.") +def test_run_prettier_on_synthax_error_file(synthax_error_json): + with pytest.raises(ValueError) as exc_info: + nf_core.lint_utils.run_prettier_on_file(synthax_error_json) + assert exc_info.value.args[0].startswith(f"Can't format {synthax_error_json} because it has a synthax error.") + + +def test_run_prettier_pre_commit_on_synthax_error_file(synthax_error_json, ensure_prettier_is_not_found): + with pytest.raises(ValueError) as exc_info: + nf_core.lint_utils.run_prettier_on_file(synthax_error_json) + assert exc_info.value.args[0].startswith(f"Can't format {synthax_error_json} because it has a synthax error.") From 5f4711e1f5819ae6f249aaef5b55f4b2c4b3851e Mon Sep 17 00:00:00 2001 From: fabianegli Date: Mon, 14 Nov 2022 14:07:29 +0100 Subject: [PATCH 11/12] remove unused imports --- tests/test_lint_utils.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/test_lint_utils.py b/tests/test_lint_utils.py index 1d864d77df..88d5553346 100644 --- a/tests/test_lint_utils.py +++ b/tests/test_lint_utils.py @@ -1,6 +1,4 @@ import shutil -from pathlib import Path -from unittest import mock import git import pytest From fe1ca2b58b18ddb85656cadacc0890de764b7e89 Mon Sep 17 00:00:00 2001 From: fabianegli Date: Thu, 17 Nov 2022 14:14:52 +0100 Subject: [PATCH 12/12] exclusiely run prettier from pre-commit hook --- nf_core/lint_utils.py | 41 +--------------------------------------- tests/test_lint_utils.py | 33 -------------------------------- 2 files changed, 1 insertion(+), 73 deletions(-) diff --git a/nf_core/lint_utils.py b/nf_core/lint_utils.py index 5f59dad8e9..b86e1a853d 100644 --- a/nf_core/lint_utils.py +++ b/nf_core/lint_utils.py @@ -1,5 +1,4 @@ import logging -import shutil import subprocess from pathlib import Path @@ -55,45 +54,7 @@ def print_fixes(lint_obj): def run_prettier_on_file(file): - """Run Prettier on a file. - - Args: - file (Path | str): A file identifier as a string or pathlib.Path. - - Warns: - If Prettier is not installed, a warning is logged. - """ - - if shutil.which("prettier"): - _run_prettier_on_file(file) - elif shutil.which("pre-commit"): - _run_pre_commit_prettier_on_file(file) - else: - log.warning( - "Neither Prettier nor the prettier pre-commit hook are available. At least one of them is required." - ) - - -def _run_prettier_on_file(file): - """Run natively installed Prettier on a file.""" - - try: - subprocess.run( - ["prettier", "--write", file], - capture_output=True, - check=True, - ) - except subprocess.CalledProcessError as e: - if ": SyntaxError: " in e.stderr.decode(): - raise ValueError(f"Can't format {file} because it has a synthax error.\n{e.stderr.decode()}") from e - raise ValueError( - "There was an error running the prettier pre-commit hook.\n" - f"STDOUT: {e.stdout.decode()}\nSTDERR: {e.stderr.decode()}" - ) from e - - -def _run_pre_commit_prettier_on_file(file): - """Runs pre-commit hook prettier on a file if pre-commit is installed. + """Run the pre-commit hook prettier on a file. Args: file (Path | str): A file identifier as a string or pathlib.Path. diff --git a/tests/test_lint_utils.py b/tests/test_lint_utils.py index 88d5553346..d5af915fae 100644 --- a/tests/test_lint_utils.py +++ b/tests/test_lint_utils.py @@ -49,50 +49,17 @@ def git_dir_with_json_syntax_error(temp_git_repo): return file -@pytest.fixture -def ensure_prettier_is_not_found(monkeypatch): - def dont_find_prettier(x): - if x == "pre-commit": - which_x = WHICH_PRE_COMMIT - elif x == "prettier": - which_x = None - else: - raise ValueError(f"This mock is only inteded to hide prettier form tests. {x}") - return which_x - - monkeypatch.setattr("nf_core.lint_utils.shutil.which", dont_find_prettier) - - -@pytest.mark.skipif(shutil.which("prettier") is None, reason="Can't test prettier if it is not available.") def test_run_prettier_on_formatted_file(formatted_json): nf_core.lint_utils.run_prettier_on_file(formatted_json) assert formatted_json.read_text() == JSON_FORMATTED -def test_run_pre_commit_prettier_on_formatted_file(formatted_json, ensure_prettier_is_not_found): - nf_core.lint_utils.run_prettier_on_file(formatted_json) - assert formatted_json.read_text() == JSON_FORMATTED - - -@pytest.mark.skipif(shutil.which("prettier") is None, reason="Can't test prettier if it is not available.") def test_run_prettier_on_malformed_file(malformed_json): nf_core.lint_utils.run_prettier_on_file(malformed_json) assert malformed_json.read_text() == JSON_FORMATTED -def test_run_prettier_pre_commit_on_malformed_file(malformed_json, ensure_prettier_is_not_found): - nf_core.lint_utils.run_prettier_on_file(malformed_json) - assert malformed_json.read_text() == JSON_FORMATTED - - -@pytest.mark.skipif(shutil.which("prettier") is None, reason="Can't test prettier if it is not available.") def test_run_prettier_on_synthax_error_file(synthax_error_json): with pytest.raises(ValueError) as exc_info: nf_core.lint_utils.run_prettier_on_file(synthax_error_json) assert exc_info.value.args[0].startswith(f"Can't format {synthax_error_json} because it has a synthax error.") - - -def test_run_prettier_pre_commit_on_synthax_error_file(synthax_error_json, ensure_prettier_is_not_found): - with pytest.raises(ValueError) as exc_info: - nf_core.lint_utils.run_prettier_on_file(synthax_error_json) - assert exc_info.value.args[0].startswith(f"Can't format {synthax_error_json} because it has a synthax error.")