-
Notifications
You must be signed in to change notification settings - Fork 82
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 pre commit #459
Add pre commit #459
Conversation
5e55d9e
to
cec50a5
Compare
cec50a5
to
4613304
Compare
what about adding GH release job, so all releases are synchronized and automated |
@laDok8 I think that's a reasonable request, to automate more of the release process for the repo, as currently its only based on tagging, and creating GH releases is optional and non-standard. I'd ask that we do that separately from this PR though, via a new issue. If you're interested it would be a great scope for you to learn more about packaging/publishing and GHA. |
thanks, i would love to 👍 |
9cb847a
to
13d8627
Compare
@@ -0,0 +1,40 @@ | |||
ci: | |||
autofix_prs: false |
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.
its potentially helpful to leave this, eases the life for drive by contributors that just messed up syntax
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 would leave it set to false
as contributors might not realize that someone/something else pushed new commits to their branch. I would expect contributors to have pre-commit installed locally, therefore they should run the checks before pushing the commit - either as an actual git hook or manually by pre-commit run
.
There is still the possibility to fix it by adding a pre-commit.ci autofix
comment to the PR. See https://pre-commit.ci/#configuration-autofix_prs
pre-commit.ci autofix |
Add pre-commit config with some basic hooks Use pre-commit.ci
remove python version spec
d05d4e6
to
82bd26f
Compare
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.
ACK on @mshriver's patch.
I have rebased his patch and ran pre-commit. I would like to get at least one more ACK before we merge it.
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 explicitly spelling out the project choice
it fits with the expectations
Depends on #458