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

Add version to github action (and rewrite the whole thing while at it) #1940

Conversation

stefanfoulis
Copy link
Contributor

@stefanfoulis stefanfoulis commented Jan 22, 2021

Allows setting the exact black version use with the github action.
This is useful to be able to avoid needing to deal with black formatting changes introduced in new black versions in unrelated pull-requests.

@stefanfoulis stefanfoulis force-pushed the feature/allow-overriding-black-version-in-github-action branch from 9e58dce to 8af883a Compare January 22, 2021 23:53
@ichard26
Copy link
Collaborator

ichard26 commented Mar 4, 2021

@stefanfoulis are you still working on this? No worries if you are no longer so, just checking in.

@ichard26 ichard26 added the C: integrations Editor plugins and other integrations label Mar 6, 2021
@stefanfoulis stefanfoulis force-pushed the feature/allow-overriding-black-version-in-github-action branch from da50fbb to 55a46b8 Compare April 6, 2021 12:48
@stefanfoulis
Copy link
Contributor Author

@ichard26 I made some changes to make the tests pass. I suppose we should also add some tests, but I don't know how to test github actions with multiple configurations.

@ichard26 ichard26 self-requested a review April 6, 2021 21:55
@ichard26
Copy link
Collaborator

ichard26 commented Apr 6, 2021

@stefanfoulis thanks! I'll take a look eventually (disclaimer: "eventually" may take a while!). For the testing, I usually just use a spare GitHub repository to test in (e.g. https://github.com/ichard26/testblackaction) but I've heard of https://github.com/nektos/act which allows you to run GHA workflows locally (can't say if it's any good since I haven't used it, but it looks promising at a few quick glances!).

Copy link
Collaborator

@cooperlees cooperlees left a comment

Choose a reason for hiding this comment

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

Sorry about missing this. Lets rebase + fix the help message. I'm happy for this to have no change log since it does not break the main black program itself.

action.yml Outdated Show resolved Hide resolved
action.yml Show resolved Hide resolved
@ichard26
Copy link
Collaborator

FYI, I'll take over and push this PR to completion @stefanfoulis, thanks for the initial work!

@ichard26 ichard26 self-assigned this May 16, 2021
@ichard26
Copy link
Collaborator

ichard26 commented May 16, 2021

I've hacking on this and I just realized that since now this is a composite action, dependency clashes are possible between Black's dependencies and whatever is installed by the rest of the containing workflow.

While a composite action does mean this action can be used on any OS and doesn't require a hacky workaround (and I don't even know if that works), the lack of isolation might be an issue (dependencies clashes, requires the host to have Python 3.6+ as the python pip is attached to).

@cooperlees WDYT?

@cooperlees
Copy link
Collaborator

cooperlees commented May 17, 2021

I've hacking on this and I just realized that since now this is a composite action, dependency clashes are possible between Black's dependencies and whatever is installed by the rest of the containing workflow.

Maybe make a venv for black so we can never have dependency clashes?

@ichard26 ichard26 force-pushed the feature/allow-overriding-black-version-in-github-action branch from bcc22fe to c27ccb4 Compare May 18, 2021 21:12
@ichard26 ichard26 marked this pull request as ready for review May 18, 2021 21:13
@ichard26 ichard26 marked this pull request as draft May 18, 2021 21:14
@ichard26 ichard26 force-pushed the feature/allow-overriding-black-version-in-github-action branch from 16223d3 to 4e246d8 Compare May 19, 2021 20:05
Since we're moving to a composite based action, quite a few changes
were made. 1) Support was added for all OSes (Windows was painful).
2) Isolation from the rest of the workflow had to be done manually
with a virtual environment.

Other noteworthy changes:

- Rewrote basically all of the logic and put it in a Python script
  for easy testing (not doing it here tho cause I'm lazy and I can't
  think of a reasonable way of testing it).
- Renamed `black_version` to `version` to better fit the existing
  input naming scheme.
- Added support for log groups, this makes our action's output a
  bit more fancy (I may or may have not added some debug output too).
Reflect compatability and gotchas.
@ichard26 ichard26 force-pushed the feature/allow-overriding-black-version-in-github-action branch from 4e246d8 to 4881c7b Compare May 29, 2021 20:40
@ichard26 ichard26 marked this pull request as ready for review May 29, 2021 20:44
@ichard26
Copy link
Collaborator

Example runs:

@ichard26 ichard26 changed the title Add black_version to github action Add version to github action (and rewrite the whole thing while at it) May 29, 2021
@ichard26 ichard26 removed their request for review May 29, 2021 20:52
action.yml Outdated Show resolved Hide resolved
@ichard26 ichard26 linked an issue May 30, 2021 that may be closed by this pull request
Copy link
Collaborator

@cooperlees cooperlees left a comment

Choose a reason for hiding this comment

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

Nice work. Just some nit picks / suggestions for you to take or leave. Thanks for all the work here. Great move going to Python rather than using bash :D

action/main.py Outdated Show resolved Hide resolved
action/main.py Show resolved Hide resolved
action/main.py Outdated Show resolved Hide resolved
docs/integrations/github_actions.md Show resolved Hide resolved
@ichard26 ichard26 requested a review from cooperlees May 31, 2021 21:28
@ichard26
Copy link
Collaborator

Thanks @JelleZijlstra and @cooperlees for the reviews, they are super appreciated :)

Hopefully this is ready to go now @cooperlees!

@ichard26 ichard26 merged commit 4005246 into psf:main Jun 1, 2021
@ichard26
Copy link
Collaborator

ichard26 commented Jun 1, 2021

Thank you @stefanfoulis so much! While I may have been the person to push this to completion, without your initial work, I don't think we would've ever worked on this feature (or atleast for a quite a while!). I'm so happy with the end result, much better usability now. If you have any cycles to spare, it would be great if you could provide some feedback from your experience contributing to psf/black, more details here: #2238.

Thank you once again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: integrations Editor plugins and other integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow picking a specific black version in the github action
4 participants