DNM Added Black formatting check action for GitHub

[ianmkenney]

I've added a basic format checking action (using Black). This is somewhat related to #291, but this PR is for a GitHub action versus local commit hooks that are managing more than just formatting.

The addition of pre-commit-config.yaml in the PR linked above only appears useful locally, but I assume the plan is to hook into https://pre-commit.ci/ at some point since expecting new developers to always remember to install the hooks is unrealistic and thus unenforceable without checks like the one added in this PR.

Generally, I'm in favor of keeping external services to a minimum, with particular emphasis on those that write commits. Alternatively, we can add individual actions for each check. It might instead make sense to maintain an OpenFreeEnergy Python package action that is easy to include in any new Python projects.

I'm setting this as DNM for now to give time for discussion. Thoughts @IAlibay @mikemhenry @dotsdl? Please ping others if you think they'd be interested!

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.73%. Comparing base (e57f300) to head (dce30f0).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #363   +/-   ##
=======================================
  Coverage   98.73%   98.73%           
=======================================
  Files          36       36           
  Lines        2049     2049           
=======================================
  Hits         2023     2023           
  Misses         26       26           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mattwthompson]

Not a stakeholder but IME https://pre-commit.ci/ has been better than a separate action

• Lightning quick (even quicker when the linters using Rust tools instead of those written in pure Python!), generally completing before I've navigated from my terminal to the web page
• Keeps all configs in one place - for all humans and all robots
• No/minimal config separate from what's used to run checks locally
• Changes are only introduced when the config changes, whereas a pip install black reliably breaks once or more times a year and/or requires human updates
• Automatically updates (with control over frequency)
• Can easily "attribute" linting diffs to a bot

[IAlibay]

We should discuss this at the next gufe meeting - I suspect whatever approach we take here is something we will want to enforce across the ecosystem.

To be honest I increasingly start to take @ianmkenney's stance here - precommit's workflow often gets in my way / leads to unecessary commits. I will go with the majority, but I like the cleanliness of a black action just telling me off on diff.

[mikemhenry]

@ianmkenney

The addition of pre-commit-config.yaml in the PR linked above only appears useful locally, but I assume the plan is to hook into https://pre-commit.ci/ at some point since expecting new developers to always remember to install the hooks is unrealistic and thus unenforceable without checks like the one added in this PR.

Yes that is the plan, I like using https://pre-commit.ci/ since it means someone can contribute to the project without having to setup the hooks

Generally, I'm in favor of keeping external services to a minimum, with particular emphasis on those that write commits. Alternatively, we can add individual actions for each check. It might instead make sense to maintain an OpenFreeEnergy Python package action that is easy to include in any new Python projects.

This is actually why I like pre-commit over github actions, if I install the hooks then I can run them locally, but you can't run a github action locally.

+1 To everything @mattwthompson said

@IAlibay

To be honest I increasingly start to take @ianmkenney's stance here - precommit's workflow often gets in my way / leads to unecessary commits. I will go with the majority, but I like the cleanliness of a black action just telling me off on diff.

How does it git in your way? If you make a commit and push it and pre-commit reformats it (and you forget to run git pull (which to be clear I often do)) so then on your next push you get an error about things diverging, just wack in a --force -- normally this is a BadIdea ™️ if you are working with another person actively, but in this case it will push your commit and pre-commit will reformat everything again. If the changes are not drastic a git merge should also be able to fix it, but the --force always will :)

Since we don't pay by the commit, I am not sure what makes them unnecessary, we squash our PRs so if a PR had 100 or 10 commits, it gets turned into a single commit when merged in.

A reason I don't like just having a black --diff tell me that my code needs formatting is all I am going to do is run black manually, so I might as well have a robot fix it for me. It also saves us from having to comment on PRs "Hey thanks for the PR, all the test pass but can you run black".

Also, while I know this discussion is focused on black, there are a lot of hooks that I am not sure have 1-1 parity with actions, like a hook that rejects a commit if it contains a huge file or checks yaml syntax for example.

[ianmkenney]

This is actually why I like pre-commit over github actions, if I install the hooks then I can run them locally, but you can't run a github action locally.

Don't get me wrong, I love pre-commit hooks, I use them extensively in all of my projects but set them up manually and strictly as validators that will never stage anything. I'm sure you can set up the pre-commit framework config to do that, but I've not looked nor tried. If the config file makes it easier for others to use and set up locally, I'm all for it. My concern is managing a service where I don't have absolute control over its behavior wrt my code. For instance is there a way to ignore the bots checks if I needed to, say, commit a binary file that is larger than whatever cutoff is in the config?

How does it git in your way? If you make a commit and push it and pre-commit reformats it (and you forget to run git pull (which to be clear I often do)) so then on your next push you get an error about things diverging, just wack in a --force -- normally this is a BadIdea ™️ if you are working with another person actively, but in this case it will push your commit and pre-commit will reformat everything again. If the changes are not drastic a git merge should also be able to fix it, but the --force always will :)

I think the above statement demonstrates how it gets in the way.

Since we don't pay by the commit, I am not sure what makes them unnecessary, we squash our PRs so if a PR had 100 or 10 commits, it gets turned into a single commit when merged in.

It doesn't look like squashing is enforced right now (see main history). I suppose this is somewhere in the repo settings to force it.

It also saves us from having to comment on PRs "Hey thanks for the PR, all the test pass but can you run black".

This is a valid point, but I figured a red X next to formatting would communicate that message just as effectively (throw in a "code submitted in this PR has been formatted according to the project styling guidelines" to the PR template for good measure).

DX is obviously highly subjective. I'm fine going in whichever direction here (with an obvious preference ;)), but the conversation hadn't happened yet and I assume it will set a standard that will be laid out in the Software Development Practices document for OpenFE projects.

[mikemhenry]

Just to address this point:

My concern is managing a service where I don't have absolute control over its behavior wrt my code. For instance is there a way to ignore the bots checks if I needed to, say, commit a binary file that is larger than whatever cutoff is in the config?

Yes! There are config options https://github.com/pre-commit/pre-commit-hooks?tab=readme-ov-file#check-added-large-files and you can also add files to the exclude list for that hook as well (as well as pattern/regex).

[IAlibay]

How does it git in your way? If you make a commit and push it and pre-commit reformats it (and you forget to run git pull (which to be clear I often do)) so then on your next push you get an error about things diverging, just wack in a --force -- normally this is a BadIdea ™️ if you are working with another person actively, but in this case it will push your commit and pre-commit will reformat everything again. If the changes are not drastic a git merge should also be able to fix it, but the --force always will :)

This is the thing, my workflow is to do smaller commits and then format after the fact. In many cases I'll do A, push, work on B, then push. What happens here is that the work I do on B often ends up being in a part of the code that's been reformatted / has diverged, so then I have to either deal with conflicts or force things.

If I'm working in isolation that's not a bad idea, but very often we do work together - and then force becomes a really bad idea. That happens enough that I wouldn't want to be force pushing as my default behaviour.

[mikemhenry]

Maybe we then can get away with using this option: https://pre-commit.ci/#configuration-autofix_prs

That would allow you to run it when you want, and since it works with a label, it is something a maintainer could add without intervention from the PR author.

[ianmkenney]

Maybe we then can get away with using this option: https://pre-commit.ci/#configuration-autofix_prs

That would allow you to run it when you want, and since it works with a label, it is something a maintainer could add without intervention from the PR author.

That sounds like a decent option! Can also add that instruction to PR templates for new contributors.

[mikemhenry]

How does this sound, I will modify #219 (or just re-make it committing only the pre-commit file so we can review the choices made more easily) as well as update the PR template to include instructions on how to run the pre-commit bot.

[ianmkenney]

How does this sound, I will modify #219 (or just re-make it committing only the pre-commit file so we can review the choices made more easily) as well as update the PR template to include instructions on how to run the pre-commit bot.

I think that will work. We can run it by the gufe dev group tomorrow and see how everyone feels about that solution!

[dotsdl]

Closing this; @mikemhenry will create a follow-up PR from consensus achieved here.