DOC: Show and correct default alignment parameters in text.py #27346
Conversation
For issue [Doc]: text alignment defaults #27345
There was a problem hiding this comment.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
lib/matplotlib/text.py
Outdated
| @@ -1002,7 +1002,7 @@ def set_horizontalalignment(self, align): | |||
|
|
|||
| Parameters | |||
| ---------- | |||
| align : {'left', 'center', 'right'} | |||
| align : {'left', 'center', 'right'}, default: left | |||
There was a problem hiding this comment.
There is no default value here as the align parameter is not optional (i.e. you can't call set_horizontalalignment() and expect that it sets the alignement to left)
There was a problem hiding this comment.
But if you don't call the setter, the value is left. How to convey that in the docs?
There was a problem hiding this comment.
The default values are (implicitly) documented in the __init__ method (class signature).
Further, matplotlib follows numpydoc conventions:
When a parameter can only assume one of a fixed set of values, those values can be listed in braces, with the default appearing first:
order : {'C', 'F', 'A'} Description of `order`.
So the explicit description of the default value is not required in this case, we'd just need to move the baseline entry for verticalalignment to the first position.
There was a problem hiding this comment.
move the
baselineentry forverticalalignmentto the first position.
Done.
| @@ -1251,7 +1251,8 @@ def set_verticalalignment(self, align): | |||
|
|
|||
| Parameters | |||
| ---------- | |||
| align : {'bottom', 'baseline', 'center', 'center_baseline', 'top'} | |||
| align : {'bottom', 'baseline', 'center', 'center_baseline', 'top'}, \ | |||
| default: baseline | |||
| """ | |||
There was a problem hiding this comment.
same as above: there is no default value here as the align parameter is not optional.
|
This is a little complicated because these do have a default value in I think we may be able to do some appending of default values where that table gets constructed (though I have not looked at all the places that code is used/all the docstrings it interacts with, which could result in confusing double
So in short, not trivial, but probably possible... would need careful consideration of how these pieces interact though (and would include changing some "public" (though potentially should be private) APIs to do it). See also #22699 which provides a mechanism for potentially adding/modifying the info that gets put into the table. (It is currently marked as "proof of concept", and has not been merged, though has had some positive response) |
@ksunden @StefRe as an interim measure, how would you feel about giving the setter methods the same defaults as those parameters? |
I think making an API change just for documentation purposes is not the way to go (also, a setter without an argument isn't a setter anymore, I'd expect such a method be called |
did change the order of allowed arguments for set_verticalalignment per @StefRe
@ksunden as @StefRe objected to that, I reverted back to the way he likes it. While setting out along your advice, I came across the mystery as to why the docstring at https://github.com/matplotlib/matplotlib/blob/main/lib/matplotlib/text.py#L125 doesn't appear anywhere in https://matplotlib.org/devdocs/api/_as_gen/matplotlib.pyplot.text.html -- so that led me to discover the top of that doc page comes from https://github.com/matplotlib/matplotlib/blob/main/lib/matplotlib/axes/_axes.py#L633 so I clarified the alignment parameter defaults there instead of the table. |
Because this is where the header at https://matplotlib.org/devdocs/api/_as_gen/matplotlib.pyplot.text.html comes from.
|
Review please? |
… parameters in text.py
|
Thanks @jsalsman and congratulations on your first merged PR! Hope to see you around! |
…346-on-v3.8.x Backport PR #27346 on branch v3.8.x (DOC: Show and correct default alignment parameters in text.py)
For issue [Doc]: text alignment defaults: closes #27345
PR summary
Show the default text
horizontalalignment/ha(left) andverticalalignment/va(baseline) in the parameter docstrings (and the associated gallery page), and correct an error falsely claiming thatbottomwas the vertical default.PR checklist