Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
DOC: Show and correct default alignment parameters in text.py#27346
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
For issue [Doc]: text alignment defaults#27345
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.
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 uson gitter for real-time discussion.
For details on testing, writing docs, and our review process, please seethe developer guide
We strive to be a welcoming and open project. Please follow ourCode 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There is no default value here as thealign
parameter is not optional (i.e. you can't callset_horizontalalignment()
and expect that it sets the alignement toleft
)
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.
But if you don't call the setter, the value isleft
. How to convey that in the docs?
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 default values are (implicitly) documented in the__init__
method (class signature).
Further, matplotlibfollowsnumpydoc 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 thebaseline
entry forverticalalignment
to the first position.
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.
move the
baseline
entry forverticalalignment
to 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
same as above: there is no default value here as thealign
parameter is not optional.
This is a little complicated because thesedo have a default value in Ithink 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) |
jsalsman commentedNov 22, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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
jsalsman commentedNov 23, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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 athttps://github.com/matplotlib/matplotlib/blob/main/lib/matplotlib/text.py#L125 doesn't appear anywhere inhttps://matplotlib.org/devdocs/api/_as_gen/matplotlib.pyplot.text.html -- so that led me to discover the top of that doc page comes fromhttps://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 athttps://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.xBackport PR#27346 on branch v3.8.x (DOC: Show and correct default alignment parameters in text.py)
Uh oh!
There was an error while loading.Please reload this page.
For issue[Doc]: text alignment defaults:closes#27345
PR summary
Show the default text
horizontalalignment
/ha
(left
) andverticalalignment
/va
(baseline
) in the parameter docstrings (andthe associated gallery page), and correct an error falsely claiming thatbottom
was the vertical default.PR checklist