Skip to content

Enable linting rule FA102 (future-required-type-annotation) #13182

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

Merged
merged 4 commits into from
Jan 26, 2025

Conversation

notatallshaw
Copy link
Member

@notatallshaw notatallshaw commented Jan 26, 2025

This catches a common error when developing for pip, which is that you use PEP 585- or PEP 604-style type annotations because your local environment is Python 3.11+, but pip currently supports back to Python 3.8.

I have often done this myself, and it usually gets caught in CI when running the test suite, but with this rule it would get caught much earlier, either locally in pre-commit, or as part of the pre-commit CI test.

Note, that while the rule documentation suggests using from __future__ import annotations it is not marked as safe so Ruff will not automatically suggest it, and currently pip coding style is to simply avoid PEP 585- and PEP 604-style type annotations.

This PR is waiting for #13181 to land, as I didn't want to clash with that PR.

@notatallshaw
Copy link
Member Author

pre-commit is failing as expected: https://results.pre-commit.ci/run/github/1446467/1737909284.hT9ZchAGSnaZaupyq0hYqw

Once #13181 lands I will merge from main and mark ready for review.

@ichard26 ichard26 marked this pull request as ready for review January 26, 2025 17:15
Copy link
Member

@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.

Thank you!

@notatallshaw notatallshaw merged commit f5ff4fa into pypa:main Jan 26, 2025
11 checks passed
@notatallshaw
Copy link
Member Author

Apologies if I was little eager merging, I read the sequence of events as this was implicitly accepted by @sbidoul, and only hesitated afterwards when I realized it wasn't explicitly accepted.

@ichard26
Copy link
Member

ichard26 commented Jan 26, 2025

If anyone needs to be attacked for eagerly merging, that would be me :)

PRs that only influence pip's development generally need less review than PRs that impact our users. Of course, if it's a major linting change (e.g, reformatting the project with ruff) when you should wait for commentary and approvals, ideally.

When considering how to treat a PR, my ballpark is to plot somewhere against the "potential impact" (scope of user-facing changes and complexity) and "controversialness" (will someone disagree with the change's design or implementation)

--- Low impact High impact
Not controversial, *yawn* Quick merge cycle (if the PR is sitting for too long, a self-merge is probably fine) Wait until several approvals, with a minimum time for commentary
Potentially controversial Wait until at least one approval, with a minimum time for commentary Wait until several approvals, with a long minimum time for commentary

"minimum time for commentary" varies from PR to PR, but I like to wait for at least a day to a week (and as evident, we wait even longer for large PRs) depending on the scope.

This is a round-about way to say the merge was totally fine IMO.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 11, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants