Skip to content

meta: Remove black GH action#1339

Merged
sl0thentr0py merged 2 commits into
masterfrom
remove-black
Feb 14, 2022
Merged

meta: Remove black GH action#1339
sl0thentr0py merged 2 commits into
masterfrom
remove-black

Conversation

@sl0thentr0py

@sl0thentr0py sl0thentr0py commented Feb 14, 2022

Copy link
Copy Markdown
Member

Since we moved to pre-commit in #1315 anyway, we don't need this flaky black stuff to run on push.
The lint step checks with black --check either way.

@BYK

BYK commented Feb 14, 2022

Copy link
Copy Markdown
Member

@sl0thentr0py you still need someone with repo admin access to remove that required format check in branch protection rules.

@antonpirker

Copy link
Copy Markdown
Contributor

I am also for removing it.

One thing we might keep in mind: people submitting PRs maybe do not have pre-commit installed so can submit horribly formatted code.

@sl0thentr0py

sl0thentr0py commented Feb 14, 2022

Copy link
Copy Markdown
Member Author

I wanted to bring this up in the pre-commit PR but anyway, let's do it now.
We already have make format here, now that we added pre-commit, that's redundant.
I initially thought why add a new tool when we can just run make format in a simple git pre-commit hook?
But anyway, now that we added it, we can just change the Makefile and make users setup pre-commit in CONTRIBUTING.md.

@BYK

BYK commented Feb 14, 2022

Copy link
Copy Markdown
Member

One thing we might keep in mind: people submitting PRs maybe do not have pre-commit installed so can submit horribly formatted code.

@antonpirker I tried that, your linter yells at me so nothing to worry about 🤣

@sl0thentr0py
sl0thentr0py enabled auto-merge (squash) February 14, 2022 18:09
@sl0thentr0py
sl0thentr0py merged commit 6649e22 into master Feb 14, 2022
@sl0thentr0py
sl0thentr0py deleted the remove-black branch February 14, 2022 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants