Skip to content
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

Implement pre-commit hooks. #3

Merged
merged 5 commits into from
Apr 5, 2024
Merged

Conversation

owenlittlejohns
Copy link
Member

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:

  • Trailing whitespace
  • Blank lines at the end of the file.
  • Valid JSON and YAML syntax.
  • ruff linting.
  • black code formatting

Why haven't I added mypy? So far all these checks have left the service code untouched. I think we should totally add both mypy, but just wanted to get the ball rolling here.

Jira Issue ID

N/A - this is an IP-ish bit of fun.

Local Test Steps

  • Pull this branch.
  • Follow the new instructions in the README to install pre-commit.
  • Make some minor change, use git add then git commit. You should see the checks all run when you are trying to use git commit.

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.
  • Tests added/updated and passing. (Well, the hooks are all passing, but that's not quite the same thing)
  • Documentation updated (if needed).

@@ -1,19 +1,19 @@
# See https://pre-commit.com for more information
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, shoot. You already did this! Gaaah. Are you fine with just black instead of darker? Do you have a preference. Sorry to miss this!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I can tell when black-jupyter pre-commit runs, it will clean up an entire file. That could lead to a lot of git blames on unrelated changes.

I'm all for blacking these up, but should we consider including a full black commit on each repo that is excluded from git blame?

Something like this https://black.readthedocs.io/en/stable/guides/introducing_black_to_your_project.html#avoiding-ruining-git-blame

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this approach - thanks for talking through it with me on Tuesday. Here's my plan:

  • I'll preemptively squash the current commits into a single one (because Squash and Merge isn't an option here). I know it's a bit bleugh to force push once stuff is in PR, but I think this time makes sense, where the later commits can't be squashed.
  • I'll use black over all Python files (including test files). This will be it's own commit, for clarity.
  • Then I'll add the secondary commit to ignore the black commit when using git blame (this is really cool by the way).
  • Then I'll push up and we'll be set. 🤞

Other things:

  • I feel like if there is any change in the service code, we probably should have an associated patch release, because even though the code should be unchanged functionally, I feel like we need to test this in isolation. Does this sound reasonable, or am I just being paranoid?
  • Later work should, in the near future, add these checks to CI/CD, as we discussed at some point in the last couple of days.
  • I'll try to prioritise HyBIG, because I think that's the repository where you'll be working next.

Does that all sound good?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That all sounds great to me. I was trying to get the black checks into the workflows but with internet and planning needs, I couldn't do it. Def want to do that next. I may sneak it in next time I work on any of these projects.

I think that plan is fine as long as you don't squash merge this PR.

Release question? IDK either way, I would do it because I'm lazy and there are no code changes, but I don't mind having one.

Actually we're changing code, even if it's just black formatting. So a patch is warranted.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We talked about this later and you did a patch release.

# https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view

# Black code formatting of entire repository
c75775b10b19a361fd8ecfdaccc2bb2f820ea115
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check this commit hash matches the one for that implements black across the whole repo. I'm super paranoid I've got the wrong one!

@owenlittlejohns
Copy link
Member Author

Just preserving this here:

There are incompatibilities between black and pycodestyle. A future goal is to make the same black and ruff checks run in the CI/CD. When we do that, I think we should nix the pylint and pycodestyle checks in favour of black and ruff.

Copy link
Member

@flamingbear flamingbear left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me. I'm excited

rev: 24.3.0
hooks:
- id: black-jupyter
args: ["--skip-string-normalization"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't want to commit?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You didn't get to see this.

@@ -1,19 +1,19 @@
# See https://pre-commit.com for more information
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That all sounds great to me. I was trying to get the black checks into the workflows but with internet and planning needs, I couldn't do it. Def want to do that next. I may sneak it in next time I work on any of these projects.

I think that plan is fine as long as you don't squash merge this PR.

Release question? IDK either way, I would do it because I'm lazy and there are no code changes, but I don't mind having one.

Actually we're changing code, even if it's just black formatting. So a patch is warranted.

@@ -1,19 +1,19 @@
# See https://pre-commit.com for more information
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We talked about this later and you did a patch release.

0, 0, 255, 255,
225, 100, 25, 25,
0, 0, 0, 0
255,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hate black here. I wonder if it has a way to be skipped?
yes thanks stack overflow.

        # fmt: off
        palette_sequence = [
            255, 0, 0, 255,
            0, 255, 0, 255,
            0, 0, 255, 255,
            225, 100, 25, 25,
            0, 0, 0, 0
        ]
        # fmt: on

I would like that please.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did it myself

@owenlittlejohns owenlittlejohns merged commit a931233 into main Apr 5, 2024
3 checks passed
@owenlittlejohns owenlittlejohns deleted the implement-pre-commit-hooks branch April 5, 2024 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants