Don't tie pyupgrade changes to dropping of support for a Python version

[pfmoore]

See #13185 (comment) and following comments for background.

Currently, we include ruff's "pyupgrade" rules in our default configuration. This means that any code which doesn't follow those rules will fail our lint checks. The auto-fixer will deal with such issues, and for day to day use this is perfectly fine. However, when we drop support for a particular Python version, this has a number of undesirable consequences:

1. Dropping support includes a potentially large change consisting of the pyupgrade fixes. Review of this commit should be straightforward, as it's all automatically generated, but it's probably mixed in with other commits doing other parts of the "drop support" exercise. Also, there's nothing that requires the pyupgrade changes to be isolated in a single commit, so it's possible for unrelated changes to slip in¹, and get missed in the review.
2. By tying the switch to using newer features to the dropping of support, we increase the impact of dropping support. Why deliberately break users when we don't immediately need to? Furthermore, if the pyupgrade changes are part of the "drop support" PR, reverting just the pyupgrade changes becomes more complex (either revert the whole PR dropping support, or cherry pick the specific commit, if there is one).

I would prefer it if our policy was to create a separate PR to apply changes recommended by pyupgrade, at some point after a Python version has been desupported. I'm not concerned if it's immediately after, or after some time has passed - that's a judgement that can be made based on the size and impact of the change. What matters is that the automated changes from pyupgrade are in a separate PR, independent of the "drop support" PR.

If that means we have to disable the pyupgrade checks in our ruff configuration, then I'm OK with that. But equally, if it's possible somehow to have ruff warn about then without making them must-fix problems, I'd be fine with that as well. I'm not sufficiently familiar with our linting setup to know if that's an option.

Footnotes

1. I'm assuming by accident, but there's also the potential for malicious changes - the xz utils backdoor isn't something we can ignore. ↩

[notatallshaw]

My only strong opinion is that some pyupgrade rules should be enabled as part of regular CI, it prevents contributors from using depricated aliases or styles, e.g. UP005, UP020, UP032, etc.

I would prefer it if our policy was to create a separate PR to apply changes recommended by pyupgrade, at some point after a Python version has been desupported.

One thing that could be done to do this without turning off pyupgrade rules is to set the minimum Python version in ruff configuration directly: https://docs.astral.sh/ruff/settings/#target-version. Then the pyupgrade rules won't kick in when project.requires-python is changed.

[notatallshaw]

One thing that could be done to do this without turning off pyupgrade rules is to set the minimum Python version in ruff configuration directly: https://docs.astral.sh/ruff/settings/#target-version.

This was done in #13238, I don't know if it's worth documenting somewhere for future PRs that drop Python support?

[pfmoore]

Probably. I'm not sure where would be a good place, though - we don't really have any documentation around our linter setup or style guidelines, as far as I know 🙁

[sbidoul]

I think the comment we have in pyproject.toml is sufficient documentation for future PRs that drop Python support.

# Pinned to delay pyupgrade changes (https://github.com/pypa/pip/issues/13236)
target-version = "py38"  # Pin Ruff to Python 3.8

[notatallshaw]

Does anyone have an option on when to update Ruff's target-version?

Ruff has been adding a number of preview rules that catch Syntax Errors which can't be turned off, and test_proxy.py uses some Python 3.9 only syntax:

$ ruff check . --preview --ignore ALL
tests/functional/test_proxy.py:28:10: SyntaxError: Cannot use parentheses within a `with` statement on Python 3.8 (syntax was added in Python 3.9)
   |
26 |     script: PipTestEnvironment, capfd: pytest.CaptureFixture[str]
27 | ) -> None:
28 |     with (
   |          ^
29 |         proxy.Proxy(port=0, num_acceptors=1) as proxy1,
30 |         proxy.Proxy(plugins=[AccessLogPlugin], port=0, num_acceptors=1) as proxy2,
   |

tests/unit/test_req_file.py:1036:14: SyntaxError: Cannot use parentheses within a `with` statement on Python 3.8 (syntax was added in Python 3.9)
     |
1034 |         # it's hard to rely on a locale definitely existing for testing
1035 |         # so patch things out for simplicity
1036 |         with (
     |              ^
1037 |             caplog.at_level(logging.WARNING),
1038 |             mock.patch("locale.getpreferredencoding", return_value=locale_encoding),
     |

tests/unit/test_req_file.py:1069:14: SyntaxError: Cannot use parentheses within a `with` statement on Python 3.8 (syntax was added in Python 3.9)
     |
1067 |         req_file.write_bytes(data)
1068 |
1069 |         with (
     |              ^
1070 |             pytest.raises(UnicodeDecodeError),
1071 |             mock.patch("locale.getpreferredencoding", return_value=encoding),
     |

Found 3 errors.

So once these rules are stabilized we will need to choose:

1. Set target-version to py39 and run pyupgrade
2. Set target-version to py39 and revert pyupgrade to previous smaller set
3. Keep target-version at py38 and revert to Python 3.8 syntax

I don't have a strong opinion about choosing any specific one, but I would like one to be picked.

If there is a strong need by downstream distributors to use new versions of pip with old versions of Python (e.g. RHEL still supports Python 3.6, do they keep patching / updating pip?) and we want to make life easier for them, we could keep target-version at py38 until things start to break on a new version of Python.

[pradyunsg]

Does anyone have an option on when to update Ruff's target-version?

I'm guessing you meant opinion (not option). My opinion is that we should do so ~now.