Use `pre-commit run prettier` if `prettier` is not available #1983

fabianegli · 2022-10-29T11:06:39Z

This is a POC for a proposal in #1972 and should be discussed before being merged.

This PR is introduces the ability to use Prettier on systems that don't have it installed already - via the use of the prettier pre-commit hook available from the nf-core pre-commit configuration.

Of specific interest for discussion is the move of pre-commit from being a development dependency to being a production dependency.

I was not able to test this PR thoroughly - so it would be nice if someone who is more experienced with pipeline/module developers to experiment.

It would probably be easy from here on out to run the pre commit hooks by default after each file mutation that the tools make. This could then also run all or a selection of the other hooks in the config.

This contains the changes currently in #1974

ToDo

Maybe remove support for non-pre-commit Prettier installations
Install the necessary pre-commit hooks upon installation of nf-core tools

Notes:

@ewels noted that the logging could be more informative: Refactor prettier invocation #1974 (comment)
@awgymer hadd additional feature ideas: Refactor prettier invocation #1974 (comment)

PR checklist

This comment contains a description of changes (with reason)
CHANGELOG.md is updated
If you've fixed a bug or added code that should be tested, add tests!
Documentation in docs is updated

codecov · 2022-10-29T11:14:18Z

Codecov Report

Merging #1983 (0ec2d1e) into dev (eb19d3f) will increase coverage by 0.09%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev    #1983      +/-   ##
==========================================
+ Coverage   67.41%   67.51%   +0.09%     
==========================================
  Files          42       42              
  Lines        5463     5467       +4     
==========================================
+ Hits         3683     3691       +8     
+ Misses       1780     1776       -4

Impacted Files	Coverage Δ
nf_core/lint_utils.py	`85.71% <100.00%> (+12.03%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

pinin4fjords · 2022-10-31T11:02:18Z

So, I tried this out as follows in the module repo, with the tools repo installed from your fork/branch. Is this what you expect @fabianegli ? I'm guessing the first commit shouldn't fail that way?

Test steps:

Installed pre-commit config from the tools repo:

> pre-commit install
pre-commit installed at .git/hooks/pre-commit

Did the module creation and observed the unpretty pytest_modules.yml:

> nf-core modules create foo/bar 
...
INFO     Created / edited following files:                                                                                                                                                                                                                                                              create.py:276
           ./modules/nf-core/foo/bar/main.nf                                                                                                                                                                                                                                                                         
           ./modules/nf-core/foo/bar/meta.yml                                                                                                                                                                                                                                                                        
           ./tests/modules/nf-core/foo/bar/main.nf                                                                                                                                                                                                                                                                   
           ./tests/modules/nf-core/foo/bar/test.yml                                                                                                                                                                                                                                                                  
           ./tests/modules/nf-core/foo/bar/nextflow.config                                                                                                                                                                                                                                                           
           ./tests/config/pytest_modules.yml     

> git diff ./tests/config/pytest_modules.yml
diff --git a/tests/config/pytest_modules.yml b/tests/config/pytest_modules.yml
index 80c3ce85..745855fa 100644
--- a/tests/config/pytest_modules.yml
+++ b/tests/config/pytest_modules.yml
@@ -1,2881 +1,2885 @@
 abacas:
-  - modules/nf-core/abacas/**
-  - tests/modules/nf-core/abacas/**
+- modules/nf-core/abacas/**
+- tests/modules/nf-core/abacas/**
...

Committed the changes on my branch:

> git add modules/nf-core/foo tests/modules/nf-core/foo 
> git commit -m "Test add module"  modules/nf-core/foo tests/modules/nf-core/foo tests/config/pytest_modules.yml 
black................................................(no files to check)Skipped
isort................................................(no files to check)Skipped
prettier.................................................................Failed
- hook id: prettier
- files were modified by this hook

tests/config/pytest_modules.yml
modules/nf-core/foo/bar/meta.yml


python tests naming..................................(no files to check)Skipped

Observed that files didn't commit, but that fix has been applied:

> git diff tests/config/pytest_modules.yml 
diff --git a/tests/config/pytest_modules.yml b/tests/config/pytest_modules.yml
index 80c3ce85..dfba19b8 100644
--- a/tests/config/pytest_modules.yml
+++ b/tests/config/pytest_modules.yml
@@ -931,6 +931,10 @@ flye:
   - modules/nf-core/flye/**
   - tests/modules/nf-core/flye/**
 
+foo/bar:
+  - modules/nf-core/foo/bar/**
+  - tests/modules/nf-core/foo/bar/**
+
 fq/lint:
   - modules/nf-core/fq/lint/**
   - tests/modules/nf-core/fq/lint/**

> git status
On branch nf-core-tools-testing
Changes to be committed:
  (use "git restore --staged <file>..." to unstage)
	new file:   modules/nf-core/foo/bar/main.nf
	new file:   modules/nf-core/foo/bar/meta.yml
	new file:   tests/modules/nf-core/foo/bar/main.nf
	new file:   tests/modules/nf-core/foo/bar/nextflow.config
	new file:   tests/modules/nf-core/foo/bar/test.yml

Re-run the commit again and see it work:

git commit -m "Test add module"  modules/nf-core/foo tests/modules/nf-core/foo tests/config/pytest_modules.yml
black................................................(no files to check)Skipped
isort................................................(no files to check)Skipped
prettier.................................................................Passed
python tests naming..................................(no files to check)Skipped
[nf-core-tools-testing 16074d74] Test add module
 6 files changed, 162 insertions(+)
 create mode 100644 modules/nf-core/foo/bar/main.nf
 create mode 100644 modules/nf-core/foo/bar/meta.yml
 create mode 100644 tests/modules/nf-core/foo/bar/main.nf
 create mode 100644 tests/modules/nf-core/foo/bar/nextflow.config
 create mode 100644 tests/modules/nf-core/foo/bar/test.yml

nf_core/lint_utils.py

of elif

fabianegli · 2022-11-14T16:34:10Z

@pinin4fjords Thanks again for testing this PR.

For anyone wanting to tag along:

Setting everything up:

#!/bin/bash

set -Eeuxo pipefail

mkdir test_pre_commit_prettier
cd test_pre_commit_prettier
gh repo clone fabianegli/tools
pushd tools
gh pr checkout https://github.com/nf-core/tools/pull/1983
python3.7 -m venv venv
. ./venv/bin/activate
pip install -e .
popd
gh repo clone nf-core/modules
cd modules
pre-commit run prettier --config ../tools/.pre-commit-config.yaml --all-files

Creating a new module:

nf-core modules create foo/bar
git add modules/nf-core/foo tests/modules/nf-core/foo tests/config/pytest_modules.yml

press "n" for the biocontainer question and use the defaults for everything else.

pre-commit run prettier --config ../tools/.pre-commit-config.yaml --all-files
git diff

After #2021 is merged, the git diff should be empty.

edmundmiller

Could we just add a nf-core lint setup or just instruct users to run

pre-commit install

And include pre-commit as a dependency in tools?

edmundmiller · 2022-11-14T16:47:52Z

nf_core/lint_utils.py

+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


I'd vote we either go all in on pre-commit for nf-core tools or not.

And include pre-commit as a dependency in tools?

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.

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.

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.

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.

because pre-commit has to pull the hook from the web the first time it runs.

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)

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.

How/where does running pre-commit prettier get it's config? Although the primary use-case for tools is working with the public nf-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 running prettier (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 issue

The hooks take the configuration in the repository where they are run into account.

edmundmiller · 2022-11-14T17:05:54Z

nf_core/lint_utils.py

+        subprocess.run(
+            ["pre-commit", "run", "--config", nf_core_pre_commit_config, "prettier", "--files", file],
+            capture_output=True,
+            check=True,
        )


Just wondering if we can import pre-commit the package and call this manually? Or is using it as a subprocess more transparent.

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.

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.

If you want to know why I classify the alternative as a "hack", compare the complexity of the parameters in the run function wit the much simpler command line invocation.

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.

fabianegli · 2022-11-17T10:19:19Z

Note to Slack about the plan to drop support for running non-pre-commit hook Prettier installations.

ewels

LGTM 👍🏻

fabianegli added 3 commits October 29, 2022 12:25

use f-string instead of format

fbb1759

use pre-commit installed Prettier if not already installed

b1882d8

ship and use the nf-core pre-commit config for linting

a9a9be9

fabianegli added the WIP Work in progress label Oct 29, 2022

Merge branch 'dev' into pre-commit-run-prettier

f77c935

fabianegli mentioned this pull request Oct 29, 2022

Refactor prettier invocation #1974

Merged

4 tasks

fix typo in variable name

5860858

Merge branch 'dev' into pre-commit-run-prettier

0cca4ce

awgymer reviewed Nov 2, 2022

View reviewed changes

nf_core/lint_utils.py Outdated Show resolved Hide resolved

fabianegli and others added 11 commits November 5, 2022 21:37

simplify conditions and utilize lazy eval

1e46a5d

of elif

Merge branch 'dev' into pre-commit-run-prettier

d46eec9

fix typo in variable name

d648284

remove unused argument

d143da6

only compute fix flags if needed

dcb8ac0

Merge branch 'dev' into pre-commit-run-prettier

cd1b05d

Merge branch 'dev' into pre-commit-run-prettier

81b0ed2

make running prettier return better error messages

9c95c5c

add tests for prettier invocations

22819d9

Merge branch 'dev' into pre-commit-run-prettier

d52d83d

remove unused imports

5f4711e

edmundmiller self-requested a review November 14, 2022 16:46

edmundmiller reviewed Nov 14, 2022

View reviewed changes

fabianegli mentioned this pull request Nov 14, 2022

Adhere to Prettier formatting #2021

Merged

1 task

edmundmiller reviewed Nov 14, 2022

View reviewed changes

fabianegli added 3 commits November 15, 2022 09:29

Merge branch 'dev' into pre-commit-run-prettier

b674a3b

Merge branch 'dev' into pre-commit-run-prettier

4ad7812

Merge branch 'dev' into pre-commit-run-prettier

6760188

fabianegli added 2 commits November 16, 2022 08:47

Merge branch 'dev' into pre-commit-run-prettier

1e76772

Merge branch 'dev' into pre-commit-run-prettier

333c918

fabianegli and others added 4 commits November 17, 2022 14:14

exclusiely run prettier from pre-commit hook

fe1ca2b

Merge branch 'dev' into pre-commit-run-prettier

0d52e0d

Merge branch 'dev' into pre-commit-run-prettier

bb5ad2d

Merge branch 'dev' into pre-commit-run-prettier

0ec2d1e

ewels approved these changes Nov 29, 2022

View reviewed changes

ewels merged commit a24a562 into nf-core:dev Nov 29, 2022

fabianegli removed the WIP Work in progress label Nov 29, 2022

mashehu mentioned this pull request Nov 30, 2022

Always format modules.json after writing it #2074

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `pre-commit run prettier` if `prettier` is not available #1983

Use `pre-commit run prettier` if `prettier` is not available #1983

fabianegli commented Oct 29, 2022 •

edited

Loading

codecov bot commented Oct 29, 2022 •

edited

Loading

pinin4fjords commented Oct 31, 2022 •

edited

Loading

fabianegli commented Nov 14, 2022 •

edited

Loading

edmundmiller left a comment

edmundmiller Nov 14, 2022

fabianegli Nov 14, 2022 •

edited

Loading

edmundmiller Nov 14, 2022

fabianegli Nov 14, 2022

fabianegli Nov 14, 2022

fabianegli Nov 15, 2022 •

edited

Loading

fabianegli Nov 15, 2022

awgymer Nov 17, 2022

fabianegli Nov 17, 2022

edmundmiller Nov 14, 2022

fabianegli Nov 14, 2022

fabianegli Nov 14, 2022 •

edited

Loading

fabianegli Nov 15, 2022 •

edited

Loading

fabianegli Nov 15, 2022 •

edited

Loading

fabianegli commented Nov 17, 2022

ewels left a comment

Use pre-commit run prettier if prettier is not available #1983

Use pre-commit run prettier if prettier is not available #1983

Conversation

fabianegli commented Oct 29, 2022 • edited Loading

ToDo

PR checklist

codecov bot commented Oct 29, 2022 • edited Loading

Codecov Report

pinin4fjords commented Oct 31, 2022 • edited Loading

fabianegli commented Nov 14, 2022 • edited Loading

edmundmiller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabianegli Nov 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabianegli Nov 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabianegli Nov 14, 2022 • edited Loading

Choose a reason for hiding this comment

fabianegli Nov 15, 2022 • edited Loading

Choose a reason for hiding this comment

fabianegli Nov 15, 2022 • edited Loading

Choose a reason for hiding this comment

fabianegli commented Nov 17, 2022

ewels left a comment

Choose a reason for hiding this comment

Use `pre-commit run prettier` if `prettier` is not available #1983

Use `pre-commit run prettier` if `prettier` is not available #1983

fabianegli commented Oct 29, 2022 •

edited

Loading

codecov bot commented Oct 29, 2022 •

edited

Loading

pinin4fjords commented Oct 31, 2022 •

edited

Loading

fabianegli commented Nov 14, 2022 •

edited

Loading

fabianegli Nov 14, 2022 •

edited

Loading

fabianegli Nov 15, 2022 •

edited

Loading

fabianegli Nov 14, 2022 •

edited

Loading

fabianegli Nov 15, 2022 •

edited

Loading

fabianegli Nov 15, 2022 •

edited

Loading