Closed pre-commit-ci[bot] closed 8 months ago
What's stopping this from being changed to true
?
@Zeitsperre Ultimately, this would be @lwasser's call. There are two schools of thought around using the pre-commit autofix in CI:
I don't think there is a right or wrong approach, but merely a tradeoff on what is most important to the project.
oh yes thank you @willingc you articulated that well. @Zeitsperre i worry that for new contributors, having to force push can be scary and also if you don't know about force pushing it will be confusing as can having to pull down changes without knowing they have been made (if they are not watching the pr closely).
this way we can just run the bot right before we merge as maintainers and contribs don't have to worry about linting / code format.
@willingc Thanks for the rundown! I've run into that same situation in other projects. The solution I've landed on is to set things to True
by default and have the Pull Request Template and Contributing documentation mention that they should install pre-commit
when making changes.
One other thing that could help justify the False
setting is actually a security issue associated with pre-commit
: first-time contributors who are otherwise blocked from running GitHub Workflows by default can exploit pre-commit
to push changes that intentionally break the linting; This would then trigger the pre-commit.ci
to push changes and then trigger the workflows anyway. I can imagine a bad actor using that to try and leak some private tokens from an insecurely-written workflow.
It's something I discovered by accident a few months back. Perhaps I should disclose that to the developer.
EDIT: it's been mentioned before: https://github.com/pre-commit-ci/issues/issues/195
@Zeitsperre Interesting point about security (and thanks for the link to the issue on pre-commit) and one that I hadn't thought about but is definitely a possibility.
Given the current size of pyOpenSci and activity, I think for now that leaving as False makes sense.
I will fix the formatting issues in another PR.
updates: