-
Notifications
You must be signed in to change notification settings - Fork 187
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use pre-commit run prettier
if prettier
is not available
#1983
Changes from 17 commits
fbb1759
b1882d8
a9a9be9
f77c935
5860858
0cca4ce
1e46a5d
d46eec9
d648284
d143da6
dcb8ac0
cd1b05d
81b0ed2
9c95c5c
22819d9
d52d83d
5f4711e
b674a3b
4ad7812
6760188
1e76772
333c918
fe1ca2b
0d52e0d
bb5ad2d
0ec2d1e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,7 @@ | ||
import logging | ||
import shutil | ||
import subprocess | ||
from pathlib import Path | ||
|
||
import rich | ||
from rich.console import Console | ||
|
@@ -34,24 +36,25 @@ 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""" | ||
|
||
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) | ||
) | ||
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( | ||
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} {fix_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 <file>'." | ||
"Automatic fixes applied. Please check with 'git diff' and revert " | ||
"any changes you do not want with 'git checkout <file>'." | ||
) | ||
|
||
|
||
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. | ||
|
@@ -60,12 +63,55 @@ def run_prettier_on_file(file): | |
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], | ||
stdout=subprocess.DEVNULL, | ||
stderr=subprocess.DEVNULL, | ||
check=False, | ||
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. | ||
|
||
Args: | ||
file (Path | str): A file identifier as a string or pathlib.Path. | ||
|
||
Warns: | ||
If Prettier is not installed, a warning is logged. | ||
""" | ||
|
||
nf_core_pre_commit_config = Path(nf_core.__file__).parent.parent / ".pre-commit-config.yaml" | ||
try: | ||
subprocess.run( | ||
["pre-commit", "run", "--config", nf_core_pre_commit_config, "prettier", "--files", file], | ||
capture_output=True, | ||
check=True, | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just wondering if we can import pre-commit the package and call this manually? Or is using it as a subprocess more transparent. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the manual and the subprocess version serve different purposes. The automated one should make sure all autogenerated file changes are lint-compliant and the manual invocations can help after manually changing files plus installing the pre-commit hooks will protect the git revision history from content that doesn't pass linting. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I answered the wrong question. The real one I don't know. Well, I kind of know. It's not the intended use of pre-commit. Not impossible, but a hack. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you want to know why I classify the alternative as a "hack", compare the complexity of the parameters in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sidenote: running a command in subprocess adds very little overhead (about 0.04 seconds on a 10 year old machine) and there's no state or computation that we already have when we invoke it that we can share with prettier to make it run faster anyway. I would say we can drop this line of inquiry. |
||
except FileNotFoundError: | ||
log.warning("Prettier is not installed. Please install it and run it on the pipeline to fix linting issues.") | ||
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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,6 @@ | ||
black | ||
isort | ||
myst_parser | ||
pre-commit | ||
pytest-cov | ||
pytest-datafiles | ||
requests-mock | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,98 @@ | ||
import shutil | ||
|
||
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.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd vote we either go all in on
pre-commit
for nf-core tools or not.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR makes pre-commit a dependency. Installing the hooks is not necessary, they can be run directly. Installing them with
pre-commit install
just makes them run before any commit in the repo. So install is not (just) installing the tools but setting a git repo up to use them.pre-commit run
will also install the hooks but not change any settings in any git repo. The beauty of pre-commit is that it takes care of everything based on the configuration file and because we have one we can just use it.The way it is set up means the user doesn't need to handle any installation manually and neither is there a need for a separate
nf-core lint setup
command.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I'm following now. Still wondering, since it'll be installed, why allow the system
prettier
install? I get it's probably faster, less stuff installed, but more code for us to maintain.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am all for only using prettier via the pre-commit hook mechanism. The global Prettier would make the first invocation a bit quicker, because pre-commit has to pull the hook from the web the first time it runs. I also have some questions about the configuration for when prettier is invoked from a global install, because it might not see the same configs as the one from the pre-commit hooks. I'd have to look at this, too.
One more point for only using pre-commit prettier is that the code - both n production and testing - would become much simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am tempted to actually promote running pre-commit install by default. It would make the developer experience more platform independent, but it also installs pre-commit hooks in all repos nf-core is used in and this may or may not be desirable depending on the project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this also means internet access is necessary for the first time prettier is run. If a developer doesn't have internet access but prettier is installed on the machine it would help to keep the current setup, although it is more complex. (hitherto I remain ignorant about the issue of the configuration for Prettier native)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to move the discussion about the use of pre-commit install out of this PR because I see it as a separate issue that does not decrease the usefulness of this PR, but considerably complicates it and would drag the merge of this PR unnecessarily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How/where does running
pre-commit prettier
get it's config? Although the primary use-case for tools is working with the publicnf-core
repos I am mindful of the fact that people do use them internally (selfish bias here too as I do) and wonder whether there are cases where runningprettier
(or using some fixed config) is undesirable?If
pre-commit prettier
draws its config from the repo/global config files in the same way as running manually installed prettier does then this is probably less of an issueThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hooks take the configuration in the repository where they are run into account.