Add tool version into install_from_resolve arg documentation

[krishnan-chandra]

Closes #20722. Here's what the new output looks like:

./pants help black

`black` subsystem options
-------------------------

The Black Python code formatter (https://black.readthedocs.io/).
This version of Pants uses black 23.3.0 by default. Use a dedicated lockfile and the install_from_resolve option to control this.

...

I couldn't work out how to test this except by running the CLI to generate the description (see above). And that...seems to work?

[huonw]

This is awesome!

1. Can you add a brief sentence about this to the docs/notes/2.22.x.md file in the Python section? Seems like such a nice quality of life improvement to be worth calling out!
2. I notice that potentially two improvements to install_from_resolve's docs:
   1. somewhat tangential, but maybe the link could be to docs/python/overview/lockfiles#lockfiles-for-tools instead of https://www.pantsbuild.org/2.20/docs/python/overview/third-party-dependencies#lockfiles
   2. Could we extend the "If unspecified, ... default lockfile shipped with Pants." paragraph to include this version? E.g. "If unspecified, ... default lockfile shipped with Pants[, which uses {package} version {version}]." where the [] bit is optional.

[huonw]

Exploring the design here, did you consider the option of leaving this as is and overriding help in PythonToolRequirementsBase?

I guess this would make a bit of fiddle to add a new help_short: ClassVar[str | ...] (or similar) to PythonToolRequirementsBase that its help method uses?

[krishnan-chandra]

The challenge here is that subclasses of PythonToolRequirementBase can override help as well and make it a string (as you said). This would nullify any changes to help that we make in the superclass. We can work around this by adding a new field like help_short, but that feels more intrusive?

So I didn't want to interfere with any of the current functionality and cause breakage to any existing help text. It seemed simpler to just add in a new method and invoke that instead of messing with the existing structure.

[huonw]

Fair enough!

[huonw]

Hm, sorry to revisit this, but I'm a bit nervous about this approach, as it seems like we're pushing something that's somewhat of an implementation detail of PythonToolRequirementBase / the Python subsystems up to the top-level subsystem concept, side stepping the normal tools for this in Python (i.e. subclassing & overrides). All of these becomes available to third-party plugins, so exposing it has future-design/backwards-compatibility concerns (i.e. if we land this and then reconsider, we might have to go through deprecation cycles etc.).

I can think of a few options:

1. keep this approach, with a new method on the Subsystem class which lets "leaf" subclasses just set help while still giving superclasses flexibility for manipulating it
2. a new field manipulated by the Python tool helpers, that Python tool subclasses need to set
3. a class method that Python tool subclasses can call when formatting their own help text
4. omit this information about the default version from the subsystem help string and have it just in places that don't cause the subclassing fiddle, i.e. option help text like install_from_resolve.
   1. Variant: omit it for now so we can land this PR with just the parsing & display in option text and no change to the top-level help text, and then consider how to display it elsewhere as follow-up

For 2, If we were to add a new field, I think that'd mean:

• adding an field help_short (or some other name) to PythonToolRequirementBase
• updating the ~40 subclasses to swap help -> , which would be annoying but not horrible
• finessing how PythonToolRequirementBase sets its help property to the extra-formatting version, to work better with mypy + existing classes (I think it'd have to be help: ClassVar[str | Callable[[], str] = _real_help_classmethod with the type annotation)
• communicating the change for plugins in release notes, something like "subclasses of PythonToolRequirementBase and PythonToolBase can replace help with to use the default help behaviour which inserts the default tool version".

For 3, that might be something like a class method def default_version_help_text(cls) -> str and downstream classes like Black & ClangFormat etc. have to call it in their help constructors f"... {cls.default_version_help_text()}"`.

I'm somewhat inclined to 4i (i.e. split this PR up so that there's a separate PR with the change to the top-level help text), so that this PR isn't blocked in this discussion and we get to see some of the benefits of your work sooner. But, I'm unsure.

What do you think @krishnan-chandra?

[krishnan-chandra]

I agree with following 4i for now, and splitting out the changes to the top-level help text into a separate PR.

On a go-forward basis, here are my thoughts:

The current content of the PR is basically the same as approach 1 -

1. keep this approach, with a new method on the Subsystem class which lets "leaf" subclasses just set help while still giving superclasses flexibility for manipulating it

I understand the concern around the new method being public - I think making this method private could potentially help, although we'd have to call getattr on a private attribute which feels a bit icky.

2. a new field manipulated by the Python tool helpers, that Python tool subclasses need to set

This one might be best, especially if we make help_short required? That would give a clear indicator of what to set for developers adding future Python tools, and would also let us catch unset values via type-checking.

3. a class method that Python tool subclasses can call when formatting their own help text
   For 3, that might be something like a class method def default_version_help_text(cls) -> str and downstream classes like Black & ClangFormat etc. have to call it in their help constructors f"... {cls.default_version_help_text()}"`.

While this could work, I think there's some footguns here - if you're making a new Python tool, you could very easily end up calling .help instead of .default_version_help_text or miss the call entirely, which seems suboptimal.

On the whole, I'd probably vote for option 2 out of all the ones you proposed.

[krishnan-chandra]

Latest commit implements 4i and removes the tool version from top-level help text. I can implement option 2 in a follow-up PR.

[krishnan-chandra]

Also created #20912 to track the follow-up task.

[huonw]

I couldn't work out how to test this except by running the CLI to generate the description (see above). And that...seems to work?

Oh, another thought for validation: the https://pantsbuild.org docs are generated from pants help-all (https://github.com/pantsbuild/pantsbuild.org/blob/0031cf17a63bd6735491ea5016bbb596f456cee9/.github/workflows/sync_docs.yml#L33-L34), so running that and confirming it appears in the JSON output would be good to validate that it'll appear when rendered.

(If it appears in pants help black output I imagine it would, so this is just a double-check.)

[krishnan-chandra]

Okay, I think I addressed all of the items that you mentioned. The help text in help-all now looks like the below (for autoflake):

      "help": "If specified, install the tool using the lockfile for this named resolve.\n\nThis resolve must be defined in `[python].resolves`, as described in https://www.pantsbuild.org/2.22/docs/python/overview/lockfiles#lockfiles-for-tools.\n\nThe resolve's entire lockfile will be installed, unless specific requirements are listed via the `requirements` option, in which case only those requirements will be installed. This is useful if you don't want to invalidate the tool's outputs when the resolve incurs changes to unrelated requirements.\n\nIf unspecified, and the `lockfile` option is unset, the tool will be installed using the default lockfile shipped with Pants, which uses autoflake version 2.1.1.\n\nIf unspecified, and the `lockfile` option is set, the tool will use the custom `autoflake` \"tool lockfile\" generated from the `version` and `extra_requirements` options. But note that this mechanism is deprecated.",

[huonw]

This is awesome. I think it's very close to ready.

[huonw]

Fair enough!

[huonw]

Thanks for all the iteration!

[huonw]

Hm, sorry to revisit this, but I'm a bit nervous about this approach, as it seems like we're pushing something that's somewhat of an implementation detail of PythonToolRequirementBase / the Python subsystems up to the top-level subsystem concept, side stepping the normal tools for this in Python (i.e. subclassing & overrides). All of these becomes available to third-party plugins, so exposing it has future-design/backwards-compatibility concerns (i.e. if we land this and then reconsider, we might have to go through deprecation cycles etc.).

I can think of a few options:

1. keep this approach, with a new method on the Subsystem class which lets "leaf" subclasses just set help while still giving superclasses flexibility for manipulating it
2. a new field manipulated by the Python tool helpers, that Python tool subclasses need to set
3. a class method that Python tool subclasses can call when formatting their own help text
4. omit this information about the default version from the subsystem help string and have it just in places that don't cause the subclassing fiddle, i.e. option help text like install_from_resolve.
   1. Variant: omit it for now so we can land this PR with just the parsing & display in option text and no change to the top-level help text, and then consider how to display it elsewhere as follow-up

For 2, If we were to add a new field, I think that'd mean:

• adding an field help_short (or some other name) to PythonToolRequirementBase
• updating the ~40 subclasses to swap help -> , which would be annoying but not horrible
• finessing how PythonToolRequirementBase sets its help property to the extra-formatting version, to work better with mypy + existing classes (I think it'd have to be help: ClassVar[str | Callable[[], str] = _real_help_classmethod with the type annotation)
• communicating the change for plugins in release notes, something like "subclasses of PythonToolRequirementBase and PythonToolBase can replace help with to use the default help behaviour which inserts the default tool version".

For 3, that might be something like a class method def default_version_help_text(cls) -> str and downstream classes like Black & ClangFormat etc. have to call it in their help constructors f"... {cls.default_version_help_text()}"`.

I'm somewhat inclined to 4i (i.e. split this PR up so that there's a separate PR with the change to the top-level help text), so that this PR isn't blocked in this discussion and we get to see some of the benefits of your work sooner. But, I'm unsure.

What do you think @krishnan-chandra?

[huonw]

Awesome. Thank you again for the iteration