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

Add pre-commit hooks for autofixing trailing whitespace #15237

Merged
merged 3 commits into from
May 14, 2023

Conversation

AlexWaygood
Copy link
Member

No description provided.

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member Author

AlexWaygood commented May 13, 2023

(The first commit is the changes to our config files; the second commit shows the kinds of changes that these new hooks will push to PRs in CI.)

Copy link
Collaborator

@ichard26 ichard26 left a comment

Choose a reason for hiding this comment

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

The --update-data pytest flag tends to leave an extra trailing newline, but since we have pre-commit.ci autopushing fixes, it shouldn't cause more friction.

@@ -9,6 +9,7 @@ flake8-noqa==1.3.1; python_version >= "3.8" # must match version in .pre-co
isort[colors]==5.12.0; python_version >= "3.8" # must match version in .pre-commit-config.yaml
lxml>=4.9.1; (python_version<'3.11' or sys_platform!='win32') and python_version<'3.12'
pre-commit
pre-commit-hooks==4.4.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess we can add this for consistency, but I don't think people will use pre-commit-hooks as a standalone package. I didn't even know it could be run standalone until now.

Copy link
Member Author

@AlexWaygood AlexWaygood May 13, 2023

Choose a reason for hiding this comment

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

Yeah, it does seem unlikely that people are going to be manually running trailing-whitespace mypy/semanal.py locally (for example). And it's true that pre-commit requirements don't need to be pre-installed for pre-commit to work locally. So, I'm happy to remove this -- but curious to hear what others think, as well.

@@ -6,7 +6,7 @@ def get_method_signature_hook(self, fullname):
if 'FullyQualifiedTest' in fullname:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should just run Black on this?

Copy link
Member Author

@AlexWaygood AlexWaygood May 13, 2023

Choose a reason for hiding this comment

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

Black makes quite a few change if I apply this diff to our pyproject.toml file:

diff --git a/pyproject.toml b/pyproject.toml
index 3d100dff5..a06acccf2 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -23,8 +23,6 @@ target-version = ["py37", "py38", "py39", "py310", "py311"]
 skip-magic-trailing-comma = true
 force-exclude = '''
 ^/mypy/typeshed|
-^/mypyc/test-data|
-^/test-data
 '''

 [tool.isort]

I'm happy to consider doing that, but maybe that should be left for another PR? Feels like running these new hooks on our test-data/ files and running black on our test-data/ files aren't really mutually exclusive.

@github-actions
Copy link
Contributor

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

@JelleZijlstra JelleZijlstra merged commit 4490525 into python:master May 14, 2023
@AlexWaygood AlexWaygood deleted the more-hooks branch May 14, 2023 05:40
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.

3 participants