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 all linters to pre-commit. #1164

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Move all linters to pre-commit. #1164

wants to merge 12 commits into from

Conversation

sallner
Copy link
Member

@sallner sallner commented Sep 21, 2023

This is a first attempt to solve #1031. Hopefully the PR will get corrected with the current github action configuration.

I have chosen to use GHA over pre-commit.ci directly, as it is not possible to deactivate the autoupdates of the hooks in pre-commit.ci. The lite version via GHA does no autoupdates at all.

The aim would be to update the hooks only in zopefoundation/meta.

I intentionally did not run the pre-commit hook on my local machine to show the functionality on GHA. This way I also tried to separate the generated code changes with my manual ones.

Open issues/ideas:

@sallner sallner marked this pull request as draft September 21, 2023 12:37
@sallner sallner force-pushed the more-pre-commit branch 3 times, most recently from b9923a9 to c3aa228 Compare September 21, 2023 13:14
@sallner sallner requested review from dataflake, d-maurer and icemac and removed request for d-maurer September 21, 2023 15:36
@sallner sallner self-assigned this Sep 21, 2023
@sallner sallner marked this pull request as ready for review September 21, 2023 15:40
Those checkers are not able to work on single files present in a commit but need conecptionally work on the whole repository (check-manifest) and thus are not suited to be run before each commit, but only in the CI.
Copy link
Member

@icemac icemac left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, just some suggestions.

.github/workflows/pre-commit.yml Show resolved Hide resolved
src/OFS/DTMLDocument.py Outdated Show resolved Hide resolved
src/OFS/EtagSupport.py Outdated Show resolved Hide resolved
src/OFS/EtagSupport.py Outdated Show resolved Hide resolved
src/OFS/Image.py Outdated Show resolved Hide resolved
src/Shared/DC/Scripts/Signature.py Outdated Show resolved Hide resolved
src/Testing/ZopeTestCase/ZopeLite.py Outdated Show resolved Hide resolved
src/ZPublisher/tests/testHTTPResponse.py Outdated Show resolved Hide resolved
src/ZPublisher/tests/test_pubevents.py Outdated Show resolved Hide resolved
src/webdav/hookable_PUT.py Outdated Show resolved Hide resolved
sallner and others added 3 commits September 22, 2023 11:02
These are mostly documentation changes. Some of the docstrings might influence the public availability of the methods on an object.

Co-authored-by: Michael Howitz <[email protected]>
@icemac
Copy link
Member

icemac commented Sep 22, 2023

@dataflake What do you think about these changes?

@dataflake
Copy link
Member

Reviewing 260 files with a bunch of changes is insane. I am missing information what those changes are about. Is it all just changing comment formatting?

Secondly, I still don't know what the change in developer experience is. What exactly does this switch to pre-commit do? Does it now force-run stuff on every commit? Locally? Remotely? Does it make changes to my commits that I cannot control? Do I get explicit feedback what changes are made? I have no idea what this really does.

@icemac
Copy link
Member

icemac commented Aug 16, 2024

@dataflake wrote:

Reviewing 260 files with a bunch of changes is insane. I am missing information what those changes are about. Is it all just changing comment formatting?

@dataflake I am now going to split this PR into smaller chunks, which are reviewable. The changes are either about formatting or changing the code to a newer style. So the changes should not be dramatic.

Secondly, I still don't know what the change in developer experience is. What exactly does this switch to pre-commit do? Does it now force-run stuff on every commit? Locally? Remotely? Does it make changes to my commits that I cannot control? Do I get explicit feedback what changes are made? I have no idea what this really does.

Let me explain a bit:

  • To run pre-commit automatically on commits you can install the pre-commit hook into your repository (by calling pre-commit install), so it runs automatically on each git commit call. But you also can run it manually or rely on GHA to run it for you.

  • If pre-commit made any changes to your code, it will not commit the changes automatically. You will have to call git commit again with the changes included. So you in full control of the changes there is nothing done in your name you cannot control.

  • If you call git add + git commit instead of git commit -a you will can see the changes pre-commit made via git diff after it said that it made changes.

  • If you decide to let GHA run pre-commit for you, the changes it made are visible as a commit in the PR, e. g. afe29e3. This commit is also not assigned to your name but to pre-commit, so it is visible that the bot did the changes.

Was my explanation helpful?

@icemac
Copy link
Member

icemac commented Aug 16, 2024

See #1224 as the first step.

@dataflake
Copy link
Member

Like I said in #1224, I'd like some way to run those same things that GHA would run manually in my sandbox, and I don't want anything auto-committed.

@dataflake
Copy link
Member

Things are looking good. How do we move forward? @sallner mentioned a few other tools that could be integrated into pre-commit like docformatter, teyit or sphinx-lint.

@icemac
Copy link
Member

icemac commented Aug 22, 2024

I added the next steps to the description of this PR. Let me first land the PR in config/meta (including to find out what is needed so fixes can be committed by GHA (like in this PR which did not happen in zope.browserpage)), then I'll create individual PRs for the remaining tools.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants