Implement pre-commit. #6
Conversation
| @@ -16,7 +16,7 @@ ENV PYTHONDONTWRITEBYTECODE=1 | |||
| COPY tests/pip_test_requirements.txt . | |||
| RUN conda run --name hoss pip install --no-input -r pip_test_requirements.txt | |||
|
|
|||
| # Copy test directory containing Python unittest suite, test data and utilities | |||
| # Copy test directory containing Python unittest suite, test data and utilities | |||
There was a problem hiding this comment.
Woop - the trailing whitespace check picked this up and automatically fixed it (note - that fix still needs to be added/committed, so we still get to review the magical changes)
| </Dataset> |
There was a problem hiding this comment.
...and this one was from the blank line check. Good evidence that one is working, too!
| @@ -7,7 +7,6 @@ | |||
|
|
|||
| from harmony.util import config | |||
| from harmony.message import Message | |||
| from pathlib import PurePosixPath | |||
There was a problem hiding this comment.
...and lastly this one was found by ruff (an unused import).
The really good news: with our other testing (like pycodestyle) the service code was already in good shape for the ruff checks.
|
❤️! I was going to do this, but didn't get to it. You should be able to add black (or darker) and it won't change any existing code, only new changes. |
| @@ -11,6 +11,6 @@ A short description of the changes in this PR. | |||
| ## PR Acceptance Checklist | |||
| * [ ] Jira ticket acceptance criteria met. | |||
| * [ ] `CHANGELOG.md` updated to include high level summary of PR changes. | |||
| * [ ] `VERSION` updated if publishing a release. | |||
| * [ ] `docker/service_version.txt` updated if publishing a release. | |||
There was a problem hiding this comment.
@sudha-murthy spotted this in her PR for DAS-2106.
|
@flamingbear - Yeah, that's true about I still have avoided Be on the look-out for similar PRs in a couple of other repos. I've got the yen for doing this everywhere now. |
| - repo: https://github.com/psf/black-pre-commit-mirror | ||
| rev: 24.3.0 | ||
| hooks: | ||
| - id: black-jupyter |
There was a problem hiding this comment.
This is a new thing since v21.something of black. The name is a little misleading - it adds checking of Jupyter notebooks, alongside existing functionality. It won't just check Jupyter notebooks.
There was a problem hiding this comment.
I ran the above test, and I believe it ran as expected. I see that:
- When I run
pre-commit run --all-files, it automatically makes all the required format changes for me to manuallygit add. - When I make a commit, it appears to only check the files I've changed and doesn't automatically change them when they fail.
I think I was right next to this conversation between you and @flamingbear a couple days ago, but why don't we just go through and do a quick sweep of all the files in each repo to go ahead and get all our code up to par? Was there some concern about git history?
@lyonthefrog - good question. My main hesitation was that it would be a bunch of changes all at once and maybe difficult to review, where as updating as we go would clean things up in a digestible way. That said, @flamingbear won me over with the ability to add commits to ignore in the blame, so I am doing exactly this. (I did a bunch of work on it yesterday, but without internet, so it hasn't been fully tested/pushed yet). Here's the updated plan. |
owenlittlejohns
force-pushed
the
implement-pre-commit
branch
from
d846494
to
d49138b
Compare
owenlittlejohns
force-pushed
the
implement-pre-commit
branch
from
d49138b
to
8645d1d
Compare
| W504 to allow for breaking of some long lines. PEP8 suggests | ||
| breaking the line before a binary operatore is more "Pythonic". | ||
| * E203, E701: This repository uses black code formatting, which deviates | ||
| from PEP8 for these errors. |
There was a problem hiding this comment.
Description
This PR begins the process of applying automated checking to all commits via pre-commit hooks. Currently it checks a small number of things:
rufflinting.Why haven't I added
blackormypy? So far all these checks have left the service code untouched. I think we should totally add bothmypyandblack, but just wanted to get the ball rolling here. I've largely been inspired by @flamingbear wanting things like this for a while, and by thexarrayimplementation of this.Jira Issue ID
N/A - this is an IP-ish bit of fun.
Local Test Steps
pre-commit run --all-files.git addthengit commit. You should see the checks all run when you are trying to usegit commit.PR Acceptance Checklist
Jira ticket acceptance criteria met.CHANGELOG.mdupdated to include high level summary of PR changes.VERSIONupdated if publishing a release.