Write data parameter docs as regular parameter not as note (v2) #20291
Conversation
lib/matplotlib/axes/_axes.py
Outdated
| @@ -1084,6 +1086,8 @@ def vlines(self, x, ymin, ymax, colors=None, linestyles='solid', | |||
|
|
|||
| Other Parameters | |||
| ---------------- | |||
| data : indexable object, optional | |||
| # data_parameter_message | |||
There was a problem hiding this comment.
This does seem easier to me. As-is it looks like a comment rather than something that will get magically filled it. Maybe:
| # data_parameter_message | |
| # _data_parameter_placeholder |
I guess its not possible to just use our usual doc string interpolation here?
There was a problem hiding this comment.
Indeed, the # does not make much sense. I've gone with DATA_PARAMETER_PLACEHOLDER, which should reasonably look out-of-context to indicate it's not literally part of the docstring.
We cannot easily use regular docstring interpolation because the contents depends on the parameters of the @_preprocess_data decorator. I've also refrained from using string formatting, because that may interfere with other characters like { and % which may appear in the plain text.
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.
55a8dde to
d11234c
Compare
| if "data : indexable object, optional" not in docstring: | ||
| _log.debug("data parameter docstring error: no data parameter") | ||
| if 'DATA_PARAMETER_PLACEHOLDER' not in docstring: | ||
| _log.debug("data parameter docstring error: missing placeholder") |
There was a problem hiding this comment.
I think we should just throw (possibly via an assert) in that case? this would be consistent with what _preprocess_data does for invalid replace_names.
(In any case, the exception should never reach end users, only CI...)
There was a problem hiding this comment.
I agree that the message should not reach end users. Particularly, I also don't want the checks to be run when the user imports matplotlib. I figured that tying execution to debug-logging is the easiest approach. Otherwise we'd need special flags or environment variables to check. I'm not worried that a user sees the message if he runs with debug logging. Debug logging outputs various other similarly technical messages.
There was a problem hiding this comment.
If you worry about performance, I think you can simply do:
replaced = docstring.replace(..., ...)
assert replaced != docstring
return replaced(without the in checks) as the string comparison should in practice be very fast as the strings won't even have the same length (which is checked first in the C-level unicode_compare_eq). This may well be competitive with the two attribute accesses in _log.level <= logging.DEBUG :-) (well, tbh I haven't checked)
(Then you don't check that there's "data : indexable..." but I think that's fine, especially as CI also checks that the result is proper numpydoc.)
There was a problem hiding this comment.
I'm a bit hesitant with the assert. If something goes wrong, then matplotlib is not importable anymore.
And I don't think an ill-formatted docstring justifies terminating the library.
There was a problem hiding this comment.
But this should really not reach the end user; if somehow we miss this on CI then something is likely very wrong with our release process...
In any case I'm not blocking this if others agree with logging instead of asserting.
There was a problem hiding this comment.
I'm more concerned that someone outside our release process may tinker with the docstrings. E.g. someone could strip all docstrings for "performance reasons". While the specific case of a missing docstring is handled, it would break if all docstrings were replaced by "stripped". - Yes, that's an unlikely scenario. It appears to me that raising ever so slightly but unnecessarily increases the risk of breakage.
Replaces #19859. This approach uses an explicit template
in the docstring and does not try to guess a reasonable insertion
position based on the docstring content. This is much simpler.