Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
oscargus merged 9 commits intomatplotlib:mainfromjsalsman:patch-2
Dec 13, 2023
Merged

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

oscargus merged 9 commits intomatplotlib:mainfromjsalsman:patch-2
Dec 13, 2023

Conversation

jsalsman
Copy link
Contributor

@jsalsmanjsalsman commentedNov 19, 2023
edited
Loading

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

PR summary

Show the default texthorizontalalignment/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

Copy link

@github-actionsgithub-actionsbot 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 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.

jsalsman reacted with laugh emoji
@jsalsmanjsalsman changed the titleShow and correct default alignment parameters in text.py[Doc]: Show and correct default alignment parameters in text.pyNov 20, 2023
@jsalsmanjsalsman changed the title[Doc]: Show and correct default alignment parameters in text.pyDOC: Show and correct default alignment parameters in text.pyNov 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

@StefReStefReNov 21, 2023
edited
Loading

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)

jsalsman reacted with eyes emoji
Copy link
ContributorAuthor

@jsalsmanjsalsmanNov 22, 2023
edited
Loading

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?

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

jsalsman reacted with thumbs up emoji
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

move thebaseline 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
"""
Copy link
Contributor

@StefReStefReNov 21, 2023
edited
Loading

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.

jsalsman reacted with eyes emoji
@ksunden
Copy link
Member

This is a little complicated because thesedo have a default value in__init__, but not in theirset_<attr> method... but the way the docstring is constructed takes the descriptions from the setters.

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 doubleDefault: ... 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 reacted with eyes emoji

@jsalsman
Copy link
ContributorAuthor

jsalsman commentedNov 22, 2023
edited
Loading

This is a little complicated because thesedo have a default value in__init__, but not in theirset_<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

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

@jsalsman
Copy link
ContributorAuthor

jsalsman commentedNov 23, 2023
edited
Loading

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

@jsalsman
Copy link
ContributorAuthor

Review please?

@oscargusoscargus added this to thev3.8.3 milestoneDec 13, 2023
@oscargusoscargus merged commit60d2f95 intomatplotlib:mainDec 13, 2023
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull requestDec 13, 2023
@oscargus
Copy link
Member

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

jsalsman reacted with hooray emoji

rcomer added a commit that referenced this pull requestDec 13, 2023
…346-on-v3.8.xBackport PR#27346 on branch v3.8.x (DOC: Show and correct default alignment parameters in text.py)
@jsalsmanjsalsman deleted the patch-2 branchDecember 15, 2023 19:04
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@github-actionsgithub-actions[bot]github-actions[bot] left review comments

@ksundenksundenksunden approved these changes

@StefReStefReAwaiting requested review from StefRe

Assignees
No one assigned
Labels
None yet
Projects
Milestone
v3.8.3
Development

Successfully merging this pull request may close these issues.

[Doc]: text alignment defaults
4 participants
@jsalsman@ksunden@StefRe@oscargus

[8]ページ先頭

©2009-2025 Movatter.jp