-
Notifications
You must be signed in to change notification settings - Fork 39
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
chore: setup pre-commit #234
Conversation
This is passing the sanity test 🥳 ! So as long as we properly document the fact we support python >=3.7, and make the sanity tests pass, I think we can run black+isort+pyupgrade to keep the code clean and tidy! Sorry for the really large styling commit, this is again a bold move, sorry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the really large styling commit, this is again a bold move, sorry.
No worries :) The separate commits make this easy to review.
Co-authored-by: Julian Tölle <[email protected]>
- name: Install dependencies | ||
run: pip install pre-commit | ||
|
||
- name: Run pre-commit | ||
run: pre-commit run --all-files --show-diff-on-failure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jooola FYI there's an official action for this. Actually, a pair of actions: https://pre-commit.ci/lite#example-using-pre-commit-action.
Besides, it's possible to enable the hosted service for this repo and then, it shouldn't be necessary to have a duplicate GHA workflow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the links, I didn't know pre-commit.ci was working on a new GitHub action. I remember the old one being deprecated in favor of the pre-commit.ci service.
For our use case I think this is just fine, and once the new GitHub action is out of beta we might reconsider this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just use pre-commit.ci? Using GHA seems like a waste in this case, especially since the GHA resources are shared across the entire org.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no experience with pre-commit.ci, I implemented something that has been working for me for ages. But I am happy to start using it to reduce the GHA resources usage.
With that said, I recently stumbled on https://results.pre-commit.ci/run/github/244995547/1688396810.pSqkyRKhQqGD40pGmTdY1Q and I haven't figured out how to solve this issue yet.
I will take some time once I am done with the current PRs I am working on.
SUMMARY
Add pre-commit to the repository.