-
-
Notifications
You must be signed in to change notification settings - Fork 117
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
Use pre-commit #396
base: master
Are you sure you want to change the base?
Use pre-commit #396
Conversation
I'm working on addressing the merge conflicts now. |
Hey @kurtmckee Thank so much for all your prs. Amazing work 🎉 All your other prs has been merged in, so this is the last one (for now) remaining. After this one, feel free to add another pr with the ci changes if you wish, if not, just let me know :) Thanks again. |
2832abf
to
3c07540
Compare
I won't be able to set up pre-commit.ci for the repo. It's a GitHub app that you -- as the repo owner -- have to authorize. I highly recommend doing so! I added myself to the bottom of the contributors list. Have a great day! |
I saw in #290 that not only are there flake8 issues, but that pre-commit was desirable for the project. Great!
This PR introduces a number of pre-commit hooks that format and lint the code. I also manually fixed all of the flake8 issues, as well as an issue found by actionlint.
Note
I am not adding pre-commit to CI; instead, you should enable pre-commit.ci for the repo, which has the following benefits:
For these reasons, running pre-commit in GitHub Actions is not the best practice.
Closes #290
@JessicaTegner I'm going to pause contributions for now until I hear back about the existing PRs. Any other work that I might do in parallel would likely have merge conflicts with this PR, for example.
Thanks for reviewing some of my previous PRs already! 🥳