-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Proof of concept for adding kwdoc content to properties using a decorator #22699
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
Conversation
I think this looks great! Can also be used to provide information for methods with inherited doc-strings (if that cannot be solved in some other way, e.g., |
2795d83
to
3cfb209
Compare
Suggested way forward:
|
I hold this off for now, because I think it's worth to establish explicit declaration of artist properties and make the kwdoc a part of that. See #22749 for the way forward. |
3cfb209
to
ea9f904
Compare
I've put #22749 on hold and am back here. Let's take this step by step: Here is the first step that allows to explicitly set content for kwdoc entries. Side note: IMHO it's not yet clear whether we'll keep the decorator only for kwdoc in the long run or whether it will grow additional functionality. But that's ok, we can always refactor and rename later because this is private API. |
ea9f904
to
8ab74a7
Compare
This seem reasonable to me and will merge the full rollout 👍🏻 |
I guess one may even consider adding a simpler version just containing the rcparam? Like
(I guess that the rcparam name is quite related, but maybe not related enough to automatically add that?) |
That depends on the definition of "simple" 😄. I'd argue: Taking the given string and using it as the kwarg doc entry is pretty simple. Parsing the docstring (maybe stripping None (?)) and putting this plus the given string into a template string is more complex. On a general note: There's a strong believe among developers that duplication is a bad thing and to be avoided at all costs. That's not true per-se. Duplication is only harmful if it makes your code cluttered and thus less readable, or if syncronization becomes an issue - either the risk of getting out of sync or the effort need for changing all places. But synchronization is not an issue if you likely don't change in the future. On the other hand, deduplication always comes at the cost of indirection (how is this description generated?) and stronger coupling (are we sure we always want
I'm pretty sure it's not related enough. |
Maybe I wasn't completely clear. I see this as a complement to meet the "yet another thing for new contributors to consider"-argument. This would then only be used if the doc-string does not contain "or None" (i.e., is suitable to start with). It would also add the benefit of a way to directly connect the rcparams to specific setter/kwargs. For the latter part, one may even consider adding something like:
where if Maybe a bit more time consuming when generating the docs (which indeed is an aspect as well), but will enable to add things in a more flexible manner and can be used for extended things in the future. |
lib/matplotlib/_docstring.py
Outdated
@@ -3,6 +3,28 @@ | |||
from . import _api | |||
|
|||
|
|||
def kwdoc(text): |
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.
This name and description is still unclear to me, even after you explained it to me. I think the name should be more descriptive, and the docstring here more explicit about what problem this solves.
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.
The name was taken over from the function that generates uses this information to generate the docstring we insert for the kwargs:
matplotlib/lib/matplotlib/artist.py
Line 1779 in 2f8fc76
def kwdoc(artist): |
But I agree, it's a bit mysterious. Would this be better?
def kwarg_doc(text):
Decorator for defining the documentation used when artist properties are
passed through as keyword arguments.
See e.g. the `**kwargs` section in `.Axes.text`.
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 still think the above would be more clear
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.
Chaned to kwarg_doc
.
is basically a superset of the API defined here. We can add the |
IMHO this is ready as a minimal version of the feature. The usage is demonstrated in one example. Systematic application should be a follow-up PR, After the fundamental concept is approved here. Extensions can be added later when needed. This is internal API, so we can always refine or reconsider. |
241c20e
to
ee602b3
Compare
lib/matplotlib/_docstring.py
Outdated
See e.g. the `**kwargs` section in `.Axes.text`. | ||
|
||
The given text is stored in a privat variable ``_kwarg_doc`` on the method. | ||
It is used for generating the kwdoc list for artists, which we | ||
auto-generate through docstring interpolation, e.g. via | ||
``%(Line2D:kwdoc)s``. | ||
|
||
The text should contain the supported types as well as the default value | ||
if applicable, e.g.: | ||
|
||
@_docstring.kwarg_doc("bool, default: :rc:`text.usetex`") | ||
def set_usetex(self, usetex): |
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'm finding this a little hard to follow and wondering if it'd be clearer if this was streamlined to one use case - so like an example of a method that uses line2D kwargs and then a code example that applies this decorator using a line2d kwarg.
d8a91ba
to
d47f986
Compare
I also rebased and used @jklymak's documentation, with some modifications. |
So there is a "new" error in that Anyway, it would be of interest to make some progress on this and decide on which approach to proceed with. |
@oscargus I'm happy to approve this if its ready to go? |
Is this a proof of concept that should not be merged (as the title suggests) or ready to merge (as the I've mostly been dropping this down my review list because of the title. |
I think something like this should be really nice to have. Maybe we should discuss it (once more?) at a dev call before making a decision on which approach. On the other hand, it is not a big deal to merge this and then decide that we would like a slightly different approach. (The big effort is adding decorators...) |
Does this still need discussion? |
d47f986
to
a7a0a67
Compare
From my perspective, this is ready to go. |
a7a0a67
to
745992f
Compare
lib/matplotlib/_docstring.py
Outdated
|
||
See e.g. the ``**kwargs`` section in `.Axes.text`. | ||
|
||
The given text is stored in a privat variable ``_kwarg_doc`` on the method. |
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.
The given text is stored in a privat variable ``_kwarg_doc`` on the method. | |
The given text is stored in a private attribute ``_kwarg_doc`` on the method. |
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.
Should this also explicitly say it is for use on the Artist property setter, not just any random method.
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.
On the one hand I don't want to nitpick the docstring to death, on the other I wonder if clarity could be improved by not aiming for conciseness.
lib/matplotlib/_docstring.py
Outdated
It is used for generating the kwdoc list for artists, which we | ||
auto-generate through docstring interpolation, e.g. via | ||
``%(Line2D:kwdoc)s``. |
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.
The phrasing of this implies that this is already implemented via interpolation
lib/matplotlib/_docstring.py
Outdated
Decorator for defining the documentation used when artist properties are | ||
passed through as keyword arguments. |
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.
Having a little bit of trouble following this - is there a more direct way of saying what this decorator does?
My reading is that the decorator stores documentation for the Artist property and the documentation stored on the decorator is what populates interpolated fields for that Artist?
745992f
to
aaf53c9
Compare
I've rewritten the docstring to hopefully better explain what this does. |
Got merged with small typo in docstring (surprised it wasn't caught by the pre-commit :/) so fixed that. Also added a comma b/c phrase seemed parenthetical.
Got merged with small typo in docstring (surprised it wasn't caught by the pre-commit :/) so fixed that. Also added a comma b/c phrase seemed parenthetical.
PR Summary
As discussed in today's dev call. The effect is visible in the
Axes.text
docstring:Performance prognosis
I slightly underestimated the number of artist properties we have. Instead of a few hundred, we have 4119 artist properties. 😲. Anyway, using a string given via the decorator should be significantly faster than scraping the docstring for such info. I'm 99.9% certain we won't have longer import times due to the decorator mechanism.
Feedback is welcome before I start to roll this out massively.