Add optional commit hook to run black

[vsoch]

A user / developer that wants to run black automatically will likely appreciate having an optional hook. We could either anticipate this and add right away, or wait for someone to explicitly ask for it. If anyone is reading this issue and would like this supported, please share your thoughts!

[bkmgit]

Assuming this is for Python code formatting (https://github.com/psf/black), for buildtest itself it would be nice to have this run automatically when people make a pull request to update the code.

…

On Fri, Feb 21, 2020, at 10:59 PM, Vanessasaurus wrote: A user / developer that wants to run black automatically will likely appreciate having an optional hook. We could either anticipate this and add right away, or wait for someone to explicitly ask for it. If anyone is reading this issue and would like this supported, please share your thoughts! — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#174>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAM2A5PM34Q4KLXTGAATZOLREAXATANCNFSM4KZJN3TA>.

[vsoch]

We discussed this, and any kind of automated commit or PR would require extra effort on the part of maintainers to review, or in the case of a commit, a need to rebase with every push. It’s also best to take a conservative approach with GitHub actions as there is some risk in giving the bot push access to branches. As an intermediate solution, a hook to run black pre commit would satisfy all parties - the code would be formatted automatically without need for rebase, and it would be installable by those that want it (for example my preference is to not use hooks). The code is tested either way to ensure the formatting is there.

This issue is to get feedback on adding that hook, which is not done yet.

[bkmgit]

Not sure if one can run test for Black formatting (black --check) automatically upon trying to merge? If ok, code can be merged, if not ok, committer will need to format code. This saves reviewing time. Tensorflow has documentation on Style guides: https://www.tensorflow.org/community/contribute/code_style There are a number of automated checks that are run before contributions are manually reviewed.

…

On Sun, Feb 23, 2020, at 9:16 AM, Vanessasaurus wrote: We discussed this, and any kind of automated commit or PR would require extra effort on the part of maintainers to review, or in the case of a commit, a need to rebase with every push. It’s also best to take a conservative approach with GitHub actions as there is some risk in giving the bot push access to branches. As an intermediate solution, a hook to run black pre commit would satisfy all parties - the code would be formatted automatically without need for rebase, and it would be installable by those that want it (for example my preference is to not use hooks). The code is tested either way to ensure the formatting is there. This issue is to get feedback on adding that hook, which is not done yet. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#174>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAM2A5IADJD36QIRLYXWZWLREIIEZANCNFSM4KZJN3TA>.

[vsoch]

Yes we already added back —check to run before merge is allowed!

[shahzebsiddiqui]

@bkmgit I like black, so far there are no issues, the black --check is in place. I had tried this initially when i first experimented with black using GitHub action. There was a PR related to auto-black on PR by @cclauss #171 that did what I wanted, but then we realize the auto-black leads to undesired consequences as mentioned by @vsoch. My main concern is

• User has to pull & rebase after every commit that leads to autoblack
• Black causes some code-base or regression test to break
• Resetting HEAD and making push can lead to auto-black trigger.

So i think the hook option would make sense. I dont have much experience, looking at it are we talking about this link for GitHooks https://githooks.com/

I see there is a pre-commit hook https://pre-commit.com/. Not sure which ones to look at. @vsoch which one did you have in mind.

[cclauss]

My recommendation at this stage is that you create precommit.yml for black, codespell, flake8, and mypy and then create a GitHub Action that runs that precommit even if the author of the PR has not set up precommit.

[vsoch]

I would recommend that the pre-commit is okay, but not that it's run by a GitHub action, for the reasons that we've mentioned multiple times:

• redundant rebasing
• GitHub Actions should be used conservatively if the github action absolutely doesn't require push access, don't give it! We certainly don't need it here.

[cclauss]

The precommit process does not modify the code. It only checks the code so neither push access nor rebasing are needed.

[vsoch]

Yes, to be clear, the precommit process happens on the local machine. Your suggestion:

create a GitHub Action that runs that precommit even if the author of the PR has not set up precommit.

is a different thing - moving the pre-commit into a GitHub action does mean that we are using GitHub actions, am I missing something? Also to be clear - we already have black setup to run with --check - https://github.com/HPC-buildtest/buildtest-framework/blob/master/.github/workflows/black.yml - but I think perhaps I am misunderstanding you - you are saying to update this action to use the same script that (could be) used locally as a pre-commit? This would mean that it would do checks (but not edit anything). I would also say we could have a pre-commit that would run black (and actually change code).

[cclauss]

You are making the assumption that all contributors have precommit installed and configured. I am saying that this is an unsafe assumption. I am trying to ensure identical tests are run in either case. #170 (comment) describes how to run precommit checks in an Action instead of, or in addition to locally.

[vsoch]

yes I understand we can do this @cclauss , but if you just run pre-commit in an action, it doesn't do anything but fail if not everything checks out. The assumption is that you would run pre-commit and then allow the github actions to push back to some branch so the changes are kept. The example provided (unless I'm misunderstanding something) doesn't do anything but run the pre-commits again. This is reproduced in any GitHub action script that is running the same kind of check.

[vsoch]

Also, I'm not talking about installing the tool pre-commit, if you simply copy paste a file into .git/hooks named "pre-commit" (without the .sample extension) this is enough to run a hook. An interested user that didn't want to run black on their own (and then have it tested in the PR) would copy paste that file, nothing more than that :)

[vsoch]

I think we are talking about two different things - https://pre-commit.com/ is not the same as https://git-scm.com/book/en/v2/Customizing-Git-Git-Hooks. The second is native to git, no extra installs or configuration required, other than copy pasting the file. The latter is a neat wrapper around that, but does require a lot of extra setup. I err on the side of simplicity over adding all these extra tools - the repository already has quite a large number...

[shahzebsiddiqui]

putting a reminder we already merged pr #179 to test the pre-commit hook. Let's give some time for testing and close this task once we are comfortable with this method.

[shahzebsiddiqui]

I was able to test the github hook. Making a simple change to a file and running black to see what will be formatted

Shown below is a snapshot of a arithmetic operation change

ssi29@ag-mxg-hulk090> black --check --diff .
--- tests/test_inspect.py       2020-02-25 18:58:58.360360 +0000
+++ tests/test_inspect.py       2020-02-25 18:59:07.336414 +0000
@@ -18,11 +18,11 @@
 def test_distro_short():
     assert "rhel" == distro_short("Red Hat Enterprise Linux Server")
     assert "centos" == distro_short("CentOS")
     assert "suse" == distro_short("SUSE Linux Enterprise Server")
-    x=0+1*3
+    x = 0 + 1 * 3

Next I commit the file and see the following message

ssi29@ag-mxg-hulk090> git commit -m "test black on one of the regtest"
Black is installed
reformatted /mxg-hpc/users/ssi29/buildtest-framework/tests/test_inspect.py
All done! ✨ 🍰 ✨
1 file reformatted, 39 files left unchanged.
[test_black_hook 008fc62] test black on one of the regtest
 1 file changed, 1 insertion(+)
ssi29@ag-mxg-hulk090> vi /mxg-hpc/users/ssi29/buildtest-framework/tests/test_inspect.py

The commit message is from me as expected and file is updated too

ssi29@ag-mxg-hulk090> git log -1
commit 008fc62483921f004f7930f76e05405901796512
Author: Shahzeb Siddiqui <shahzebmsiddiqui@gmail.com>
Date:   Tue Feb 25 13:59:31 2020 -0500

    test black on one of the regtest

ssi29@ag-mxg-hulk090> cat tests/test_inspect.py | grep -i "x ="
    x = 0 + 1 * 3

[shahzebsiddiqui]

@bkmgit

In order for this hook to work I have to copy the pre-commit file as follows

cp .github/hooks/pre-commit .git/hooks/

This puts effort on user to do this if they want the hook feature to work. If its not done, the code will not be blacked but Git Action will detect as part of one of our checks.

[vsoch]

This is done and tested by two of us, we can open another issue if something needs to be addressed.

[shahzebsiddiqui]

I agree this is done. Thanks for closing this @vsoch