Skip to content
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

Skip autofix on PRs in pre-commit #899

Merged
merged 1 commit into from
Jul 25, 2023

Conversation

ciarams87
Copy link
Member

Proposed changes

Problem: The pre-commit check running in PRs is auto-fixing issues it finds. While that is desirable behaviour when we run pre-commit locally, it is not something we want in PRs because it alters the remote branch which can affect the developer workflow.

Solution: The pre-commit docs details a configuration option to disable this autofix feature. The solution is to add this field to the config.

Closes #898

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@ciarams87 ciarams87 requested a review from a team as a code owner July 24, 2023 16:09
@github-actions github-actions bot added the chore Pull requests for routine tasks label Jul 24, 2023
Copy link
Contributor

@kate-osborn kate-osborn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@lucacome
Copy link
Member

autofix was left on to help with minor fixes like whitespaces, especially for external people without forcing them to install pre-commit 🤔

@ciarams87
Copy link
Member Author

ciarams87 commented Jul 25, 2023

autofix was left on to help with minor fixes like whitespaces, especially for external people without forcing them to install pre-commit 🤔

@lucacome The team were discussing this offline yesterday, and having external contributors on dev branches (even if only the pre-commit bot) interrupts dev workflows - personally, I was caught out by it because my remote branch changed unexpectedly so I needed to rebase locally. External contributors don't need to necessarily install pre-commit, they can make the changes manually as detailed in the pre-commit report in the PR. If in the future it is causing issues for external contributors, we can always look at re-instating this functionality.

@ciarams87 ciarams87 merged commit 03115df into nginxinc:main Jul 25, 2023
20 checks passed
@ciarams87 ciarams87 deleted the chore/remove-autofix-pr branch July 25, 2023 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Pull requests for routine tasks
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Remove auto fixing in PRs from pre-commit
4 participants