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

10218 pre-commit autoupdate #1619

Merged
merged 6 commits into from
Jul 17, 2021
Merged

Conversation

graingert
Copy link
Member

@graingert graingert commented Jul 9, 2021

Scope and purpose

Since initially introducing pre-commit many hooks have been updated with fixes - eg pyupgrade, flake8 and black.

This PR updates those hooks and then allows pre-commit.ci to run them to demonstrate that all the changes are from real hook changes and I didn't try to slip anything hidden in with them.

Contributor Checklist:

  • The associated ticket in Trac is here: https://twistedmatrix.com/trac/ticket/10218
  • I ran tox -e lint to format my patch to meet the Twisted Coding Standard
  • I have created a newsfragment in src/twisted/newsfragments/ (see: News files)
  • The title of the PR starts with the associated Trac ticket number (without the # character).
  • I have updated the automated tests and checked that all checks for the PR are green.
  • I have submitted the associated Trac ticket for review by adding the word review to the keywords field in Trac, and putting a link to this PR in the comment; it shows up in https://twisted.reviews/ now.
  • The merge commit will use the below format
    The first line is automatically generated by GitHub based on PR ID and branch name.
    The other lines generated by GitHub should be replaced.
Merge pull request #123 from twisted/4356-branch-name-with-trac-id

Author: <github_username>, <github_usernames_if_more_authors>
Reviewer: <github_username>, <github_username_if_more_reviewers>
Fixes: ticket:<trac_ticket_number>, ticket:<another_if_more_in_one_PR>

Long description providing a summary of these changes.
(as long as you wish)

@graingert graingert changed the title pre-commit autoupdate 10218 pre-commit autoupdate Jul 9, 2021
@graingert graingert requested a review from adiroiban July 9, 2021 22:24
@adiroiban
Copy link
Member

I don't know what to say.
I would like to see a short description of this change.

What problem does it tries to solve?

With the recent supply-chain attacks I prefer to review twice before giving write access to external services.
We have code review and protected branches... so this should not be a big issue... but who knows.

Also, I am a bit worried that without auto-cancelling of the old jobs, with pre-commit.ci enabled we generate twice as many test jobs... once for the original PR push and then another one for the fixes done by pre-commit.ci

I guess that pre-commit.ci can help with nodejs/Ruby/golang project where developers don't have a python environment available.

but Twisted is based on python so it shouldn't be a big issue for Twisted developers to install and enable pre-commit and use it as a real pre-commit hook :)

At the same time, I think that core developers can enable the pre-commit locally and for that case pre-commit.ci will be a NOOP as all the pre-commit changes were already done.

I am not completely against it, on this as I guess that it might help first time contributors .

Without a PR description I can only guess why and what this PR does.

I prefer to approve the PR based on a short info from @graingert to describe what issue this PR tries to solve so that I can do a review based on that info and not based on a guess :)

I think that it would help to get at least another review from at least another Twisted developer.

@adiroiban adiroiban requested a review from a team July 10, 2021 10:46
Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

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

This can be merged...

but can you also apply in this PR the changes from https://github.com/twisted/twisted/pull/1626/files

so that we don't need a separate review.

Thanks again!

@graingert graingert merged commit b3cd393 into twisted:trunk Jul 17, 2021
@graingert graingert deleted the pre-commit-autoupdate branch July 17, 2021 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants