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

Move pre-commit GitHub action to pre-commit.ci #4263

Merged
merged 9 commits into from
Apr 15, 2023

Conversation

tkoyama010
Copy link
Member

@tkoyama010 tkoyama010 commented Apr 12, 2023

Overview

Using pre-commit official GitHub action.
Move pre-commit GitHub action to pre-commit.ci.

Details

@tkoyama010 tkoyama010 marked this pull request as ready for review April 12, 2023 02:52
@github-actions github-actions bot added the maintenance Low-impact maintenance activity label Apr 12, 2023
@tkoyama010 tkoyama010 mentioned this pull request Apr 12, 2023
@codecov
Copy link

codecov bot commented Apr 12, 2023

Codecov Report

Merging #4263 (b5e4c08) into main (07dfc66) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #4263   +/-   ##
=======================================
  Coverage   95.77%   95.77%           
=======================================
  Files          97       97           
  Lines       20755    20755           
=======================================
  Hits        19879    19879           
  Misses        876      876           

akaszynski
akaszynski previously approved these changes Apr 12, 2023
@akaszynski
Copy link
Member

The action website states:

generally you want to use pre-commit.ciwhich is faster and has more features.

I think we decided to not go with that because we didn't want to have it modify the files independently. I'm actually fine if we decide to have the CI action modify our source. I've seen this implemented in other repos and have yet to see issues arise.

@banesullivan
Copy link
Member

actually fine if we decide to have the CI action modify our source

I'd like to push back against that. If implemented, it needs to be fully opt-in.

@MatthewFlamm
Copy link
Contributor

I'm more concerned about this part

this action is in maintenance-only mode and will not be accepting new features.

This means that any change that requires significant rework will probably be the end of it.

The action website states:

generally you want to use pre-commit.ciwhich is faster and has more features.

I think we decided to not go with that because we didn't want to have it modify the files independently.

I am fairly certain that you can turn off this behavior in pre-commit.ci. It has configuration options autofix_prs which would turn off auto-updates to PRs. Auto-updates to main branch opens a PR, it doesn't directly make changes, nor should it be possible since that branch is protected. See pull requests from https://github.com/python-telegram-bot/python-telegram-bot/pulls here which do not have any autoupdate changes in PRs, but have the pre-commit.ci checks run.

@akaszynski
Copy link
Member

Thanks for the hint @MatthewFlamm!

Since pre-commit recommends for you to use their integration, I modified this PR to match python-telegram-bot/python-telegram-bot in:
https://github.com/python-telegram-bot/python-telegram-bot/blob/53093ebceb7375716840f16a794ebd028a6effbb/.pre-commit-config.yaml#L3-L5

I've disabled autofix_prs and made it 100% opt-in to match @banesullivan's request. Please note that you can run this manually on a PR by commenting with:

pre-commit.ci autofix

MatthewFlamm
MatthewFlamm previously approved these changes Apr 14, 2023
Copy link
Contributor

@MatthewFlamm MatthewFlamm left a comment

Choose a reason for hiding this comment

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

I assume an administrator to this repo will have to install the app after merging.

@MatthewFlamm
Copy link
Contributor

Oh wait it is working already 🥳

I assume we can get rid of this workflow too? https://github.com/pyvista/pyvista/blob/main/.github/workflows/pre-commit-update.yml

Or should we wait until it sends a PR first?

@tkoyama010
Copy link
Member Author

@akaszynski Thanks for finding the configuration! I had been using pre-commit-bot as the default for my repositories, but I didn't realize it could be configured in such detail.

.pre-commit-config.yaml Outdated Show resolved Hide resolved
@tkoyama010
Copy link
Member Author

I assume we can get rid of this workflow too?

@MatthewFlamm Good catch. Thanks!

@tkoyama010 tkoyama010 changed the title Using pre-commit official GitHub action Move pre-commit GitHub action to pre-commit integration Apr 15, 2023
@tkoyama010 tkoyama010 changed the title Move pre-commit GitHub action to pre-commit integration Move pre-commit GitHub action to integration Apr 15, 2023
@tkoyama010 tkoyama010 changed the title Move pre-commit GitHub action to integration Move pre-commit GitHub action to pre-commit.ci Apr 15, 2023
@tkoyama010
Copy link
Member Author

pre-commit.ci autofix

@akaszynski
Copy link
Member

pre-commit.ci autofix

Yea, I'm going to use this a lot.

@tkoyama010
Copy link
Member Author

tkoyama010 commented Apr 15, 2023

OK! All my wishes have been granted and I thank the PyVista community for help. And welcome to our community pre-commit-ci[bot] !

CONTRIBUTING.rst Outdated Show resolved Hide resolved
@akaszynski akaszynski merged commit 0c03106 into main Apr 15, 2023
20 checks passed
@akaszynski akaszynski deleted the maint/using-pre-commit-action branch April 15, 2023 05:16
@banesullivan
Copy link
Member

Okay this is quite nice... thanks for making it opt-in with the comment, that's super useful

@akaszynski akaszynski mentioned this pull request Apr 30, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Low-impact maintenance activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants