Skip to content
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

DOC: Show and correct default alignment parameters in text.py #27346

Merged
merged 9 commits into from Dec 13, 2023
Merged

DOC: Show and correct default alignment parameters in text.py #27346

merged 9 commits into from Dec 13, 2023

Conversation

jsalsman
Copy link
Contributor

@jsalsman jsalsman commented Nov 19, 2023

For issue [Doc]: text alignment defaults: closes #27345

PR summary

Show the default text horizontalalignment/ha (left) and verticalalignment/va (baseline) in the parameter docstrings (and the associated gallery page), and correct an error falsely claiming that bottom was the vertical default.

PR checklist

Copy link

@github-actions github-actions bot left a 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 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.

@jsalsman jsalsman changed the title Show and correct default alignment parameters in text.py [Doc]: Show and correct default alignment parameters in text.py Nov 20, 2023
@jsalsman jsalsman changed the title [Doc]: Show and correct default alignment parameters in text.py DOC: Show and correct default alignment parameters in text.py Nov 20, 2023
@@ -1002,7 +1002,7 @@ def set_horizontalalignment(self, align):

Parameters
----------
align : {'left', 'center', 'right'}
align : {'left', 'center', 'right'}, default: left
Copy link
Contributor

@StefRe StefRe Nov 21, 2023

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 the align parameter is not optional (i.e. you can't call set_horizontalalignment() and expect that it sets the alignement to left)

Copy link
Contributor Author

@jsalsman jsalsman Nov 22, 2023

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 is left. How to convey that in the docs?

Copy link
Contributor

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, 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move the baseline entry for verticalalignment 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
"""
Copy link
Contributor

@StefRe StefRe Nov 21, 2023

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 the align parameter is not optional.

@ksunden
Copy link
Member

ksunden commented Nov 21, 2023

This is a little complicated because these do have a default value in __init__, but not in their set_<attr> method... but the way the docstring is constructed takes the descriptions from the setters.

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 Default: ... sections if there already is one). However, it is several layers deep in how it is constructed, so may not be trivial to do so (not the least because you somehow need to thread through where to take the defaults from, and that isn't there now:

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
Copy link
Contributor Author

jsalsman commented Nov 22, 2023

This is a little complicated because these do have a default value in __init__, but not in their set_<attr> method... but the way the docstring is constructed takes the descriptions from the setters.

@ksunden @StefRe as an interim measure, how would you feel about giving the setter methods the same defaults as those parameters?

@StefRe
Copy link
Contributor

StefRe commented Nov 22, 2023

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 reset...).

did change the order of allowed arguments for set_verticalalignment per @StefRe
@jsalsman
Copy link
Contributor Author

jsalsman commented Nov 23, 2023

... 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).

@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.

@jsalsman jsalsman requested a review from StefRe November 24, 2023 20:31
@jsalsman
Copy link
Contributor Author

Review please?

@oscargus oscargus added this to the v3.8.3 milestone Dec 13, 2023
@oscargus oscargus merged commit 60d2f95 into matplotlib:main Dec 13, 2023
40 checks passed
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Dec 13, 2023
@oscargus
Copy link
Contributor

Thanks @jsalsman and congratulations on your first merged PR! Hope to see you around!

rcomer added a commit that referenced this pull request Dec 13, 2023
…346-on-v3.8.x

Backport PR #27346 on branch v3.8.x (DOC: Show and correct default alignment parameters in text.py)
@jsalsman jsalsman deleted the patch-2 branch December 15, 2023 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

[Doc]: text alignment defaults
4 participants