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

[chore] Remove black and ruff from test-requirements #18139

Merged
merged 3 commits into from
Nov 12, 2024

Conversation

sterliakov
Copy link
Contributor

#15210 standardized lint hooks invocations with pre-commit, but did not remove test requirements that became unneeded. Grepping for ruff and black revealed that they are never used directly any longer except for one outdated README notice.

Remove direct dependency on black and ruff, retaining them only in pre-commit configuration. pre-commit does not use local package installations and manages all virtual environments internally.

This comment has been minimized.

@@ -159,7 +159,7 @@ To run mypy on itself:

To run the linter:

ruff .
python3 runtests.py lint
Copy link
Collaborator

Choose a reason for hiding this comment

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

It reads a bit odd when this command uses runtests.py, and the very next line says "You can also run all of the above tests using runtests.py". It may be good to mention the underlying lint command here. FWIW, I always run pre-commit directly, and only just learned that the runtests.py invocation exists.

Suggested change
python3 runtests.py lint
pre-commit run -a

Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer to run lint using runtests.py, since it lets me perform several tasks via a single command, such as python3 runtests.py lint self pytest-fast. If we will remove black and ruff from test requirements, we should probably change runtests.py lint to run pre-commit run -a in order to avoid breaking existing workflows.

Copy link
Collaborator

Choose a reason for hiding this comment

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

we should probably change runtests.py lint to run pre-commit run -a in order to avoid breaking existing workflows.

It looks like that's already the case :-)

mypy/runtests.py

Lines 68 to 69 in 54a2c6d

# Lint
"lint": ["pre-commit", "run", "--all-files"],

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice, forgot about it!

Copy link
Contributor Author

@sterliakov sterliakov Nov 11, 2024

Choose a reason for hiding this comment

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

I think it's better to still mention python runtests.py lint here since it's difficult to discover by other means. I expanded the preceding paragraph and added pre-commit install mention - does this look better now?

(I also use pre-commit pretty much everywhere so prefer to invoke it directly)

This comment has been minimized.

Copy link
Contributor

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

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Thanks, looks good!

@JukkaL JukkaL merged commit 2ebc690 into python:master Nov 12, 2024
20 checks passed
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