added github action to auto pep8

[shimwell]

Description

This PR adds a github action that automatically formats the python code using black.

Motivation and Context

It saves developers time and effort as there would no longer be a need make commits for pep8 changes
It saves review time as the reviewer doesn't have to check the pep8 formatting

Changes

What kind of change does this PR introduce? (Bug fix, feature, documentation update...)
Automation

Behavior

What is the current behavior? What is the new behavior?
current behavior - PR is made with pep errors -> reviewer make comments in the review requesting pep8 changes -> PR contributor makes changes -> reviewer checks they are correct and accepts changes
new behavior - PR is made with pep error -> pep8 formatting is automatically corrected and committed to the PR

Other Information

Other relevant information to this pull request.
The CI is not rerun for these pep8 commits

Changelog file

All pull requests are required to update the CHANGELOG file with the PR. Your update can take different forms including:

• creating a new entry describing your change, including a reference to this pull request number
• adding a reference to your pull request number to an exist entry if that entry can include your changes

[gonuke]

Thanks @shimwell - this looks like a good addition, but probably something that will introduce a lot of changes upon merging.

A few thoughts:

1. Can we rename the action to something that includes pep8 in the name? (both the file and the action's name:) I think it will help future developers immediately know what it is
2. Perhaps it's just my control issues, but I think I would like to have a manual PR of the changes that will be introduced by this in the first instance, and then rely on it going forward as an automatic action.
3. We should update the docker images to include black rather than pip installing it here (but that could be a future PR)

[shimwell]

Thanks @shimwell - this looks like a good addition, but probably something that will introduce a lot of changes upon merging.

A few thoughts:

1. Can we rename the action to something that includes `pep8` in the name? (both the file and the action's `name:`) I think it will help future developers immediately know what it is

2. Perhaps it's just my control issues, but I think I would like to have a manual PR of the changes that will be introduced by this in the first instance, and then rely on it going forward as an automatic action.

3. We should update the docker images to include `black` rather than pip installing it here (but that could be a future PR)

Thanks @gonuke and double thanks for all the packaging work I noticed elsewhere.

1 rename 👍
2 manual PR submitted #1436 👍
3 I can do that if you like, current this github action doesn't actually use our dockerfile and installs black each time to make sure it has the latest version

[gonuke]

This looks good to me, but let me wait on #1436 before moving forward.

I do have a best practice/workflow question - this change means that a local copy of a git branch becomes out of sync with a remote copy upon pushing that copy. This makes me uneasy since a typical workflow will be:

• edit/add/commit
• push changes to fork (where now a automatic commit may occur)
• edit/add/commit
• push changes to fork (is a force push now required because things changed on the fork)
• repeat...
• submit PR
• possibly repeat some more

At each push there is the potential of things becoming misaligned.... I am sure others use autoformatting workflows like this, so I am interested in learning the best practice.

[shimwell]

Pre-commit formatting is a popular choice for devs. It does the formatting locally before the commit.
https://ljvmiranda921.github.io/notebook/2018/06/21/precommits-using-black-and-flake8/
https://levelup.gitconnected.com/raise-the-bar-of-code-quality-in-python-projects-7c49743f004f

[shimwell]

I can add a .pre-commit-config.yaml file to this PR or another PR if anyone wants

repos:
  - repo: https://github.com/psf/black
    rev: 22.1.0
    hooks:
      - id: black
  - repo: https://github.com/pre-commit/pre-commit-hooks
    rev: v4.1.0
    hooks:
      - id: check-toml
      - id: end-of-file-fixer
      - id: mixed-line-ending
      - id: trailing-whitespace

More info on pre-commit here

[gonuke]

Do I understand correctly that forcing pre-commit will require everyone to make sure they've installed some pre-commit package?

[shimwell]

Do I understand correctly that forcing pre-commit will require everyone to make sure they've installed some pre-commit package?

Developers can optionally install pre-commit with pip install pre-commit. If they do then formatting will be applied prior to a git commit. Pep8 formatting will be done locally and pushed commits will be cleaner.

If contributors don't have pre-commit installed or don't want to install it then that is not a problem and they can make a PR in the normal way. Pep8 formatting will be done by then Github Action and the commit history will have a few more commits as it will potentially have some ``[skip ci] formatting commit``` made by the action. Then the contributor may have to pull to before pushing their next commit to an open PR.

Installing pre-commit is optional, the existence of a .pre-commit-config.yaml file is just a hint to developers about the formatting preferences of the repo. It can be ignored and we can rely on the GitHub action to do the formatting automatically as a separate commit.

Happy to give a demo at the next catch up