-
-
Notifications
You must be signed in to change notification settings - Fork 606
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
Add help_short
field for Python tools and use it to generate the help text
#20921
Add help_short
field for Python tools and use it to generate the help text
#20921
Conversation
help_short
field for Python tools and use it to generate the help text
Sample output:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this separately.
I'd like to get additional eyes on this.
I think @benjyw is probably across this area the most.
# Must be set by subclasses - will be used to set the help text in this class. | ||
help_short: ClassVar[str | Callable[[], str]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine subclasses of PythonToolBase
are fairly common for people with their own custom plugins. A search like https://github.com/search?q=pants++PythonToolBase+-path%3A%2Fsrc%5C%2Fpython%5C%2Fpants%5C%2F%2F+-org%3Apantsbuild+language%3Apython&type=code indicates a bunch of ones in open source code.
In a perfect world, we wouldn't break those existing plugins with this change, i.e. so that people can upgrade easily without making changes. My theory is we don't break them, because existing classes will still override help
, thus not use this class's version (and thus not read help_short
)... but I'm not exactly sure how to verify.
Given I think it should be fine, one approach would be to land it and then see if people report problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I concur that I think this won't break.
@benjyw: background: this comes out of #20901, where @krishnan-chandra added info about the default version of each tool but within the The specific implementation approach comes out of the thread #20901 (comment) and in particular the list of options we discussed there:
The original #20901:
Thus PR then tries option 2. |
7cb3e95
to
75c2053
Compare
2cbba40
to
67d6a15
Compare
67d6a15
to
4310285
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a reasonable approach to me!
@huonw I'm fine to merge this but will wait to give you a last say. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice, thank you for iterating!
Closes #20912.
This actually wasn't too bad! I didn't realize the trick with
classproperty
would work as I intended, but it did. Looking at the individual commits should help separate out the repetitive work (changinghelp
tohelp_short
) from the more substantive changes.