Add pre-commit #40

Jorricks · 2022-03-08T08:33:18Z

Added the following tools to pre-commit:

iSort (import formatter)
Black (general formatter)
Flake8 (style checker)
Mypy (type checker)

Furthermore, modified the README.md to contain new instructions.

Two more things are left to do:

Run pre-commit on the entire codebase.
Add item to changelog.

ewels · 2022-03-08T08:45:56Z

.github/workflows/lint.yml

-        run: npm install -g prettier
-
-      - name: Run Prettier --check
-        run: prettier --check ${GITHUB_WORKSPACE}


If possible it would be nice to keep Prettier in place, for linting the markdown + YAML files.. (maybe you're already on the case for this..)

Ahhh yeah I was wondering why this was here.
My mistake, adding it back :)

~~I'll see if I can embed it into pre-commit ;)~~
Added it back as a pre-commit hook. This will help developers detect these mistakes earlier :)

No worries! Should be possible I think: https://prettier.io/docs/en/precommit.html#option-3-pre-commithttpsgithubcompre-commitpre-commit

Done ✌️ Let me know what you think :)

Jorricks · 2022-03-11T07:36:51Z

@ewels I have done all the required refactoring.
Quick summary:

Flake8 has many very restrictive rules. To loosen them up a bit, I added some exception in .flake8. Furthermore, have a couple of noqa: exceptions, but only small hand full.
MyPy has been added and with the exception of 3 lines (type: ignore), everything was fine ✌️
iSort and black worked their magic as well :)

Sorry that I didn't have more time this week to complete this earlier but I'll be off from today onwards for a week (⛷️).
If you have the time, it would be awesome to see if we can merge it today but only if we are 100% sure everything still works :). Otherwise, no rush and let's see in a week ✌️

ewels

Quickly read over the code and I think it looks great! Love how simple the GA workflow is now.

Couple of comments and would like to test the code locally before merging, but I don't see any issues so should be able to merge today 👍🏻

Kind of curious to see how well this will play with my VSCode plugins.. 😅

ewels · 2022-03-11T09:19:06Z

examples/01_simple.py

+    r"""
+    Pretend to download some files from somewhere.
+
+    Multi-line help strings are unwrapped until you use a double newline.


Intention of this example is to not have a double newline.

Suggested change

r"""

Pretend to download some files from somewhere.

Multi-line help strings are unwrapped until you use a double newline.

"""

Pretend to download some files from somewhere.

Multi-line help strings are unwrapped until you use a double newline.

Changed this back with noqas

ewels · 2022-03-11T09:27:20Z

examples/04_rich_markup.py

@@ -8,7 +8,7 @@
 @click.option(
    "--input",
    type=click.Path(),
-    help="Input [magenta bold]file[/]. [dim]\[default: a custom default][/]",
+    help=r"Input [magenta bold]file[/]. [dim]\[default: a custom default][/]",


Do we really need these r prefixes?

Suggested change

help=r"Input [magenta bold]file[/]. [dim]\[default: a custom default][/]",

help="Input [magenta bold]file[/]. [dim]\[default: a custom default][/]",

Yess flake8 was complaining about the \.. Let me just add a noqa.

ah ok, I wondered if there was a reason. The r prefix is also fine, I guess it doesn't make any difference here..

According to flake8, \[ is an invalid escape.

README.md

.pre-commit-config.yaml

ewels · 2022-03-11T09:30:55Z

.pre-commit-config.yaml

@@ -0,0 +1,58 @@
+minimum_pre_commit_version: "2.9.2"


Maybe a few comment lines at the top explaining what this file is and what it's used for, for n00bs like me?

.github/workflows/pre-commit.yml

.flake8

Jorricks · 2022-03-11T14:26:24Z

Thanks for all the good comments @ewels ✌️.
Looking forward to hear how it went when you took this branch for a spin.
As long as there are no other major PRs to be merged, I don't see any reason to rush this.

ewels

Nice changes! Couple of minor docs suggestions..

README.md

Added links Co-authored-by: Phil Ewels <phil.ewels@scilifelab.se>

Add extra information Co-authored-by: Phil Ewels <phil.ewels@scilifelab.se>

Co-authored-by: Phil Ewels <phil.ewels@scilifelab.se>

ewels · 2022-03-11T15:12:58Z

Oops, sorry about the ❌ It's amazingly difficult to write valid markdown in the GitHub PR plain text editor 😆

Note that for next time you can batch suggestions together if you want instead of doing one commit per comment. This option is only available on the Files changed PR tab though, not Overview.

ewels

Great stuff, thank you! 👏🏻

ewels · 2022-03-11T18:58:46Z

Hmm, getting this error in the actions CI:

pyproject.toml: [mypy]: Unrecognized option: scripts-are-modules = True

Any ideas?

The tests seem fairly inconsistent for me too..

Running locally flake8 was failing but not giving any error messages (GH actions was fine)
I keep fixing all the errors and pushing, then more show up.. 🤔 Is this normal?

Jorricks · 2022-03-12T04:43:06Z

Hmm, getting this error in the actions CI:
pyproject.toml: [mypy]: Unrecognized option: scripts-are-modules = True
Any ideas?

The tests seem fairly inconsistent for me too..

Running locally flake8 was failing but not giving any error messages (GH actions was fine)

I keep fixing all the errors and pushing, then more show up.. 🤔 Is this normal?

Hey @ewels,

Sorry to hear that!
The scripts are modules thing is because I forgot to rename the - to _. That should fix it.

Regarding the errors that are showing up, running pre-commit run --all should in all cases fix everything. If the behavior is still different in the pipeline then from locally, it can be two things in my experience, either the versions of the tools don't align or it's not correctly picking up the configuration of pyproject/.Flake8. I don't mind taking another look when I'm back or if you give me a particular example I can also see if I can help you debug.
It would be awesome if you could send me a screenshot and a github pipeline URL :)

ewels · 2022-03-12T05:49:21Z

Ah thanks for the tips! I'll take a look after the weekend. Thanks for the quick reply, enjoy your holiday!

Phil

Jorricks · 2022-03-12T08:19:15Z

Ah thanks for the tips! I'll take a look after the weekend. Thanks for the quick reply, enjoy your holiday!

Phil

Thanks! Have a good weekend 🤟

See #40 (comment)

ewels · 2022-03-16T00:41:23Z

Underscores typo now fixed 👍🏻

Jorricks marked this pull request as draft March 8, 2022 08:33

Jorricks changed the title ~~DRAFT Add pre-commit~~ Add pre-commit Mar 8, 2022

ewels reviewed Mar 8, 2022

View reviewed changes

Jorricks force-pushed the add-pre-commit branch from b9b83b7 to 7431372 Compare March 8, 2022 09:40

ewels mentioned this pull request Mar 8, 2022

Add PEP 561 Compatiblity #41

Merged

Jorricks force-pushed the add-pre-commit branch 2 times, most recently from b56f9ca to f4a14e4 Compare March 11, 2022 07:26

Jorricks marked this pull request as ready for review March 11, 2022 07:31

Jorricks force-pushed the add-pre-commit branch from d63247f to c182e74 Compare March 11, 2022 07:32

ewels reviewed Mar 11, 2022

View reviewed changes

Add pre-commit

89f944b

Jorricks force-pushed the add-pre-commit branch from 3c5f729 to 89f944b Compare March 11, 2022 14:16

FIX Changes according to MR

e0b8e71

ewels reviewed Mar 11, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

README.md Show resolved Hide resolved

README.md Show resolved Hide resolved

Jorricks and others added 3 commits March 11, 2022 16:09

Update README.md

3f2817a

Added links Co-authored-by: Phil Ewels <phil.ewels@scilifelab.se>

Update README.md

e3b3c1d

Add extra information Co-authored-by: Phil Ewels <phil.ewels@scilifelab.se>

Update README.md

c605783

Co-authored-by: Phil Ewels <phil.ewels@scilifelab.se>

Jorricks added 3 commits March 11, 2022 16:16

Update README.md with individual contributors

cc0d4cd

Update .pre-commit-config.yaml

cd8a4cf

FIX MR

7c3fd78

ewels approved these changes Mar 11, 2022

View reviewed changes

ewels merged commit 41451ae into ewels:main Mar 11, 2022

ewels added a commit that referenced this pull request Mar 15, 2022

Fix mypy config typo.

cb612ff

See #40 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pre-commit #40

Add pre-commit #40

Jorricks commented Mar 8, 2022 •

edited

Loading

ewels Mar 8, 2022

Jorricks Mar 8, 2022

Jorricks Mar 8, 2022 •

edited

Loading

ewels Mar 8, 2022 •

edited

Loading

Jorricks Mar 8, 2022

Jorricks commented Mar 11, 2022 •

edited by ewels

Loading

ewels left a comment

ewels Mar 11, 2022

Jorricks Mar 11, 2022

ewels Mar 11, 2022

Jorricks Mar 11, 2022

ewels Mar 11, 2022

Jorricks Mar 11, 2022

ewels Mar 11, 2022

Jorricks Mar 11, 2022

Jorricks commented Mar 11, 2022

ewels left a comment

ewels commented Mar 11, 2022

ewels left a comment

ewels commented Mar 11, 2022

Jorricks commented Mar 12, 2022

ewels commented Mar 12, 2022

Jorricks commented Mar 12, 2022

ewels commented Mar 16, 2022

	help=r"Input [magenta bold]file[/]. [dim]\[default: a custom default][/]",
	help="Input [magenta bold]file[/]. [dim]\[default: a custom default][/]",

Add pre-commit #40

Add pre-commit #40

Conversation

Jorricks commented Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jorricks Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

ewels Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jorricks commented Mar 11, 2022 • edited by ewels Loading

ewels left a 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

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

Jorricks commented Mar 11, 2022

ewels left a comment

Choose a reason for hiding this comment

ewels commented Mar 11, 2022

ewels left a comment

Choose a reason for hiding this comment

ewels commented Mar 11, 2022

Jorricks commented Mar 12, 2022

ewels commented Mar 12, 2022

Jorricks commented Mar 12, 2022

ewels commented Mar 16, 2022

Jorricks commented Mar 8, 2022 •

edited

Loading

Jorricks Mar 8, 2022 •

edited

Loading

ewels Mar 8, 2022 •

edited

Loading

Jorricks commented Mar 11, 2022 •

edited by ewels

Loading