-
-
Notifications
You must be signed in to change notification settings - Fork 8k
Write data parameter docs as regular parameter not as note #19859
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
ee8f655 to
154a139
Compare
91a27b2 to
50e52e7
Compare
50e52e7 to
ab8d409
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.
@tacaswell and I were looking at this earlier, and realized that a couple of the asserts in tests with replace_names were wrong. That text is there, but the regex is wrong.
I'm not completely sure whether this belongs in 3.4.2; I'll let you know after docs have built.
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
I'm fine with moving to 3.5 if this would hold up the 3.4.2 release.
If **kwargs* is in 'Other Parameters', then data ends up there; is that what was intended?
If
**kwargs*is in 'Other Parameters', thendataends up there; is that what was intended?
I don't have a strong opinion on this, but it's fine by me. One can argue this on a similar level as further Artist properties, reatively generic and not related to the primary semantic of the method.
Sorry for being dense - where is this change in the built docs? I can't tell from the description.
It's removing the note
image
and instead adding a regular parameter:
image
This change applies to all @_preprocess_data() decorated methods, e.g.:
https://matplotlib.org/stable/api/_as_gen/matplotlib.axes.Axes.bar.html?highlight=bar
vs.
https://58245-1385122-gh.circle-artifacts.com/0/doc/build/html/api/_as_gen/matplotlib.axes.Axes.bar.html?highlight=bar
I see, I looked at plot where data is already explicitly described so wasn't sure what you are talking about. However, is there a reason not to simply include data explicitly like plot does in the relevant methods? I know thats kind of what this does, but rather than having a machinery, but machinery is mysterious unless you know it is there.
However, is there a reason not to simply include
dataexplicitly likeplotdoes in the relevant methods?
TL;DR
_preprocess_data does multiple things, that need to match. It's not strictily necessary to have all of them in the decorator.
My main motivation here is the final formatting of the docstring. I don't have a strong opinion how that is achieved internally.
Full comment
There are three parts:
The docstring
We could make that explicit, but then might want to add a check that the mentioned parameters match with the given @_preprocess_data parameters.
An in-between way would be to
data : indexable object, optional
{data_arg_placeholder}
so that we explicitly specify the position but still auto-generate the content.
The keyword data
Could be made explicit in the signature instead of the decorator rewriting the signature.
The actual data replacement
Also this does not necessarily have to be a decorator. You could do
if data is not None:
x, y = replace_data(x, y, data)
Well, actually it's a bit more involved in general.
Thinking of it, I'd go for {data_arg_placeholder} and making data=None explicit to reduce the magic here.
Replaces matplotlib#19859. This approach uses an explicit template ``` data : indexable object, optional # data_parameter_message ``` in the docstring and does not try to guess a reasonable insertion position based on the docstring content. This is much simpler.
Replaces matplotlib#19859. This approach uses an explicit template ``` data : indexable object, optional # data_parameter_message ``` in the docstring and does not try to guess a reasonable insertion position based on the docstring content. This is much simpler.
Replaces matplotlib#19859. This approach uses an explicit template ``` data : indexable object, optional # data_parameter_message ``` in the docstring and does not try to guess a reasonable insertion position based on the docstring content. This is much simpler.
Closing in favor of #20291.
Replaces matplotlib#19859. This approach uses an explicit template ``` data : indexable object, optional # data_parameter_message ``` in the docstring and does not try to guess a reasonable insertion position based on the docstring content. This is much simpler.
Replaces matplotlib#19859. This approach uses an explicit template ``` data : indexable object, optional # DATA_PARAMETER_PLACEHOLDER ``` in the docstring and does not try to guess a reasonable insertion position based on the docstring content. This is much simpler.
Replaces matplotlib#19859. This approach uses an explicit template ``` data : indexable object, optional DATA_PARAMETER_PLACEHOLDER ``` in the docstring and does not try to guess a reasonable insertion position based on the docstring content. This is much simpler.
Replaces matplotlib#19859. This approach uses an explicit template ``` data : indexable object, optional DATA_PARAMETER_PLACEHOLDER ``` in the docstring and does not try to guess a reasonable insertion position based on the docstring content. This is much simpler.
PR Summary
It's cleaner to document this as a normal parameter rather than using a note "The function can take an additional parameter data ...".
The docs is inserted before
**kwargsdocs, or if they do not exist as the last entry inParameters(Which is the case only for a handful of methods.