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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend #186: Add GitHub Action for black formatter #200

Merged
merged 1 commit into from
Nov 24, 2019

Conversation

tomschr
Copy link
Member

@tomschr tomschr commented Nov 24, 2019

Scope

This PR is a first start of GH Actions to call the black source code formatter and does basically this:

  1. Triggered by a pull request event.
  2. Setup Python 3.7.
  3. Install black.
  4. Run black and create a diff file.
  5. Upload the diff as artifact; this can be downloaded and examined if needed. See my action run in my fork.

For the time being, it's quite "simple". This was on purpose as I've played with other fancy stuff which didn't work as expected (probably it was my fault because this topic is quite new for me).

From a design point of view, it is built upon other actions, created by GitHub itself.

At the moment, black creates a >50KB diff of the source code of all *.py files. Maybe we should create another issue to (re)format the source code with black to reduce the diff for future pull requests.

Not Implemented Ideas

I've played around in my fork to implement other more fancy tasks. However, the result were mixed. That's why I thought I start with something more simple. If wanted, we can extend it later. 馃槈

To create your own Action you either a) reuse existing Actions, or b) create your own. I've tried to start with a) first as for b) you need to know JavaScript or Docker.

I've tried the following, but didn't include it into this PR:

  • Add the diff as a comment
    I've tried to add a comment and the diff to a pull request. That would be nice so the owner of the PR would see the result from the GH Action. The comment was visible, but not the diff. For some unknown reason, I couldn't pass the result from the black step into the commenting step.

    As the diff can be quite huge, it would add a lot of "noise" to the comments. One way to avoid it would be to add only the first x lines or to use the program filterdiff. That can extract the first 2-4 "hunks" of a diff (use filterdiff -#3 project.diff).

  • Add pip caching
    Adding pip caching speeds up things. I've tested it, but it didn't work reliably for me.

Future Ideas

If we add more GH Actions to the repo in the future, it would probably interesting to create our own Actions.

As we have now a python-semver organization, it is easy to create such additional repositories under our domain. This could be source code formatting, release code management, or something else.

I think, this is probably something we should consider in the future. With our own GH Actions, we have more finer control and the Actions are separated from the semver source code.

But this is something beyond this simple PR; I've added it here to have a place for future reference.

The GH Action does basically this:
1. Setup Python 3.7
2. Install dependencies (mainly black)
3. Run black and create a diff file
4. Upload the diff as artifact

Currently, it does not any pip caching (I had some problems
with that; it didn't work reliably).
@tomschr tomschr added the Infra All about infrastructure (GitHub Action, project build etc.) label Nov 24, 2019
@tomschr tomschr self-assigned this Nov 24, 2019
@scls19fr
Copy link
Member

scls19fr commented Nov 24, 2019

LGTM

Absolutely ok for a second PR to reformat code with Black

I'm just wondering if this second PR shouldn't be merged first (letting just one or 2 minor formatting mistakes to ensure that this Github Action is working fine)

@tomschr
Copy link
Member Author

tomschr commented Nov 24, 2019

@scls19fr Thanks. 馃憤

I'm just wondering if this second PR shouldn't be merged first (letting just one or 2 minor formatting mistakes to ensure that this Github Action is working fine)

I've tested the GH Action in my fork and it creates the diff file as artifact. This artifact is only visible inside the Action tab, not in the repo itself. Or in other words: this GH Action does never change the source itself. So I think, it would be safe to merge it. :-)

What about this attempt:
If we merge it, we could see if this works (=> see the huge diff as an artifact). In a second pull request, we could format the source code and we could observe there no diff.

@scls19fr
Copy link
Member

Ok so let's merge.

@scls19fr scls19fr merged commit 9e4ebcf into python-semver:master Nov 24, 2019
@scls19fr
Copy link
Member

Maybe we should have a mention about that in CHANGELOG.
Sorry it's merged... but maybe you can it in second PR about Black formatting of current code.

@tomschr
Copy link
Member Author

tomschr commented Nov 24, 2019

Regarding the CHANGELOG: I thought about that too, but it's about the infrastructure, not the source code.

All fine! 馃憤 :-)

@scls19fr
Copy link
Member

To avoid merge conflicts maybe we should first merge #198
What is your opinion?

@tomschr
Copy link
Member Author

tomschr commented Nov 24, 2019

Yes, good idea! 馃憤

@scls19fr
Copy link
Member

If you are ok we can merge it.
And after create a PR for CHANGELOG (both for #198 and for this PR #200).
And an other one about Black

@ppkt
Copy link
Member

ppkt commented Nov 25, 2019

@tomschr Nice PR - I love this formatter :)
I know it's already merged but I have an idea - maybe instead of formatting code base and uploading .diff file, this action should only check whether code is formatted (i.e. exit code from black is 0 - see option --check)?

@tomschr tomschr deleted the feature/gh-action-black branch November 25, 2019 08:29
@tomschr
Copy link
Member Author

tomschr commented Nov 25, 2019

@ppkt Thanks for your idea! 馃憤

Yes, we can do that. 馃槈 My original idea was to comment on the corresponding pull request to let the contributor know that there is an issue with the code. However, that turned out to be a bit more difficult than I thought.

Probably, the diff is of not real use anyway. It was a start to see how we could use these GH Actions. I'll create an issue for this. Would you review it once I've implemented it? 馃槈

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Infra All about infrastructure (GitHub Action, project build etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants