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

Upgrade the default version of black from 23.12 to 24.8 #21056

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

huonw
Copy link
Contributor

@huonw huonw commented Jun 13, 2024

This upgrades the default black version from 23.3.0 to 24.8.0.

This results in "user-visible" formatting changes: we're ticking over into a new year, and thus pick up the "2024" stable style in 24.1.0: https://black.readthedocs.io/en/stable/change_log.html#id18 . Depending on the codebase, this potentially results in significant reformatting (and does so for Pants).

To make it easier for people to upgrade, this retains a copy of the old lockfile, that can be referenced via a resources: URI.

There's various other changes in the release notes, like support for new syntax, bug fixes and performance improvements.

Because Pants uses its own black backend this PR also reformats the world, according to the new style.

The commits are independently sensible:

  1. Upgrade black itself
  2. Adjust the flake8 rules due to a new conflict (https://black.readthedocs.io/en/stable/guides/using_black_with_other_tools.html#e701-e704)
  3. Reformat the whole world
  4. Various fixes after review

@@ -60,6 +60,8 @@ The backend linter will also load a Trufflehog [configuration file](https://gith

#### Python

[The `pants.backend.python.lint.black` backend](https://www.pantsbuild.org/2.23/reference/subsystems/black) now uses version 24.4.2 by default, upgrading from 23.3.0. This comes with two notable visible changes: there's a new stable style (see [release notes for 24.1.0](https://black.readthedocs.io/en/stable/change_log.html) for details) which may result in extensive reformatting, and black now requires Python 3.8 or newer to _run_ (but can still format code from older versions). To override Pants' default version, use [the `install_from_resolve` option](https://www.pantsbuild.org/2.23/reference/subsystems/black#install_from_resolve) and/or [the `interpreter_constraints` option](https://www.pantsbuild.org/2.23/reference/subsystems/black#interpreter_constraints).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

(see [release notes for 24.1.0](https://black.readthedocs.io/en/stable/change_log.html) for details)

NB. this doesn't link to the release notes heading directly because they're just numbered links from the top, not permalinks, e.g. https://black.readthedocs.io/en/stable/change_log.html#id18

This means a direct link will be wrong/misleading by the time we release Pants 2.23, if black does any releases in that time, since the 18th heading won't be 24.1.0 any more.

psf/black#4344

@huonw huonw marked this pull request as draft June 13, 2024 00:36
@huonw huonw force-pushed the huonw/upgrade-black branch from cdf52c6 to a38eaaf Compare June 13, 2024 00:48
@huonw huonw marked this pull request as ready for review June 13, 2024 01:10
@huonw huonw requested review from kaos, benjyw and thejcannon June 13, 2024 01:13
@benjyw
Copy link
Contributor

benjyw commented Jun 13, 2024

This is very disruptive to users - they upgrade Pants and suddenly all their code reformats...

I guess that is just how Black is, and we should go along with it?

@huonw
Copy link
Contributor Author

huonw commented Jun 13, 2024

Yes, agreed. I was trying to alleviate some of the issue with the release notes call out, but we might want to do more leg work.

As you say, this is how black is, and I don't think pants should be imposing its own ideas on tools, thus getting stuck on really old versions...

Just brainstorming some ideas:

  1. build infrastructure for Python tools similar to "known versions" for external tools, so Pants could have several built-in lockfiles that can be chosen between more simply than creating a whole custom resolve
  2. turn Black into an external tool somehow, e.g. have a parallel repo that publishes pexes for it
  3. have separate backends for each version of black, e.g. a pants.backend.python.lint.black23, pants.backend.python.lint.black24 then black25, with the current pants.backend.python.lint.black => pants.backend.python.lint.black23
  4. more visible warnings/callouts about the change than just release notes
  5. maybe you can think of other things?

I'm inclined to either 1 or 4, because as you say, that's just how black is... Thoughts?

@lilatomic
Copy link
Contributor

I think that if we shipped both lockfiles, we could make rolling back be as simple as creating a resolve that points to the lockfile using the internal syntax (resource://), something like

[python.resolve]
black = "resource://pants/backend/python/lint/black/black23.lock"
[black]
install_from_resolve = "black"

@kaos
Copy link
Member

kaos commented Jun 13, 2024

It would be nice if it was less involved to override which version of black to use. i.e. provide the version and pants takes care of the locking and what-not behind the scenes. But I think that is for another issue, and here, the disruption comes from black, and I'd say we don't want to get stuck on an increasingly old version for the sake of shielding users from that. Also, there is a way out of it--so we're not imposing anything impossible.

I like @lilatomic's idea as middle ground to offer an alternative built-in lockfile. But I don't know how well that plays with the change in ICs.. or if it's enough to set those along with the lockfile in the config?

Copy link
Contributor

@benjyw benjyw left a comment

Choose a reason for hiding this comment

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

Yeah, agreed. Live by the tool, die by the tool...

@huonw
Copy link
Contributor Author

huonw commented Jun 13, 2024

I think that if we shipped both lockfiles, we could make rolling back be as simple as creating a resolve that points to the lockfile using the internal syntax (resource://), something like

Nice idea, I'll have a play with that and see if it seems like a viable "ease the transition" path.

@huonw
Copy link
Contributor Author

huonw commented Jun 14, 2024

Nice, that seems to work well, running the following config (with PANTS_SOURCE=... on this branch) uses the old 23.3 version and the thus the old formatting style.

[GLOBAL]
pants_version = "2.21.0"
backend_packages = ["pants.backend.python", "pants.backend.python.lint.black"]

[python]
interpreter_constraints = ["==3.10.*"]
enable_resolves = true

[python.resolves]
black = "resource://pants.backend.python.lint.black/black-23.3.lock"

[black]
install_from_resolve = "black"

I've pushed this and rewritten the release notes to explain the upgrade path.

I'm thinking we could then deprecate this lockfile in Pants 2.24, and remove in Pants 2.26 (or something like that).

What do you think? Is it worth it?

@benjyw
Copy link
Contributor

benjyw commented Jun 15, 2024

I think we can even deprecate in 2.23, and remove in 2.24. That would be more in the spirit of Black's apparent upgrade strategy of "just disrupt the world". But we don't need to deprecate the lockfile's existence, just the relying on it by default?

@huonw
Copy link
Contributor Author

huonw commented Jun 16, 2024

Hm, I'm not 100% sure I understand your idea. Are you suggesting this?

  • In pants 2.23:
    • still use black 23 by default, but emits deprecation warnings
    • we encourage users to (explicitly) switch to black 24 somehow, potentially with a resources://.../black24.lock lockfile packaged
  • In pants 2.24:
    • switch to black 24 by default
    • retain the black 23 lockfile for a user to use explicitly, if required

Or maybe something else?

@benjyw
Copy link
Contributor

benjyw commented Jun 17, 2024

Hm, I'm not 100% sure I understand your idea. Are you suggesting this?

  • In pants 2.23:

    • still use black 23 by default, but emits deprecation warnings
    • we encourage users to (explicitly) switch to black 24 somehow, potentially with a resources://.../black24.lock lockfile packaged
  • In pants 2.24:

    • switch to black 24 by default
    • retain the black 23 lockfile for a user to use explicitly, if required

Or maybe something else?

Yes, this. Or we can get rid of the black 23 lockfile if we prefer. I'm just saying that we don't have to.

@huonw
Copy link
Contributor Author

huonw commented Sep 11, 2024

We've just branched for 2.23, so merging this pull request now will come out in 2.24, please move the release notes updates to docs/notes/2.24.x.md. Thank you!

@cburroughs
Copy link
Contributor

@huonw At this point, are you leaning towards trying to land this in 2.25?

@huonw
Copy link
Contributor Author

huonw commented Dec 18, 2024

Ah, hm, I lost track of this one. I'll get it refreshed in the next few days.

@huonw huonw changed the title Upgrade the default version of black from 23.3.0 to 24.4.2 Upgrade the default version of black from 23.12 to 24.8 Dec 19, 2024
@huonw
Copy link
Contributor Author

huonw commented Dec 19, 2024

I've updated this PR and written a (probably too large) upgrade guide in the release notes. Suggestions for how to cut it down most welcome!

Thanks for the prompt

@huonw huonw requested a review from cburroughs December 19, 2024 08:30
Copy link
Contributor

@cburroughs cburroughs left a comment

Choose a reason for hiding this comment

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

Thanks! Left some minor optional comments.

@@ -89,13 +89,11 @@ def lockfile_name(self) -> str:


@dataclass
class PythonTool(Tool[PythonToolRequirementsBase]):
...
class PythonTool(Tool[PythonToolRequirementsBase]): ...
Copy link
Contributor

Choose a reason for hiding this comment

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

Tangential rant: Oh that's rather ugly, but I guess that's on brand for black.


[The `pants.backend.python.lint.black` backend](https://www.pantsbuild.org/2.25/reference/subsystems/black) now uses version 24.8.0 by default, upgrading from 23.12.1. This comes with a new stable style (see [release notes for 24.1.0](https://black.readthedocs.io/en/stable/change_log.html) for details) which may result in extensive reformatting.

To override Pants' default version, use [the `install_from_resolve` option](https://www.pantsbuild.org/2.25/reference/subsystems/black#install_from_resolve) and/or [the `interpreter_constraints` option](https://www.pantsbuild.org/2.25/reference/subsystems/black#interpreter_constraints). The style changes may be extensive, so, to make upgrading to Pants 2.25 easier, we provide the old lockfile as built-in, for now (Pants may remove this in future, so you should either switch to your own lockfile, or upgrade to the default Black 24). To use this lockfile, and remain on Black 23.3, configure a resolve as follows:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
To override Pants' default version, use [the `install_from_resolve` option](https://www.pantsbuild.org/2.25/reference/subsystems/black#install_from_resolve) and/or [the `interpreter_constraints` option](https://www.pantsbuild.org/2.25/reference/subsystems/black#interpreter_constraints). The style changes may be extensive, so, to make upgrading to Pants 2.25 easier, we provide the old lockfile as built-in, for now (Pants may remove this in future, so you should either switch to your own lockfile, or upgrade to the default Black 24). To use this lockfile, and remain on Black 23.3, configure a resolve as follows:
To override Pants' default version, use [the `install_from_resolve` option](https://www.pantsbuild.org/2.25/reference/subsystems/black#install_from_resolve) and/or [the `interpreter_constraints` option](https://www.pantsbuild.org/2.25/reference/subsystems/black#interpreter_constraints). The style changes may be extensive, so, to make upgrading to Pants 2.25 easier, we provide the old lockfile as built-in, for now (Pants remove this in a future release, so you should either switch to your own lockfile, or upgrade to the default Black 24). To use this lockfile, and remain on Black 23.3, configure a resolve as follows:

Copy link
Contributor

Choose a reason for hiding this comment

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

At the limit eventually the CPython version compatible with the old black version will be EoL. We can't support it forewever.

install_from_resolve = "black"
```

To take control of your Black version independent of Pants' default, configure a resolve similar to the following, and generate the lockfile with `pants generate-lockfiles --resolve=black`:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
To take control of your Black version independent of Pants' default, configure a resolve similar to the following, and generate the lockfile with `pants generate-lockfiles --resolve=black`:
To take control of your Black version independent of Pants' default, configure a resolve similar to the following, and generate the lockfile with `pants generate-lockfiles --resolve=your-resolve-name`:

Copy link
Contributor

Choose a reason for hiding this comment

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

I waffle on this, not sure what makes for the better example naming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants