-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
PR: Port Figure docstrings to numpydoc #9777
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
Conversation
lib/matplotlib/figure.py
Outdated
['aitoff' | 'hammer' | 'lambert' | 'mollweide' | \ | ||
'polar' | 'rectilinear'], optional | ||
projection : {'aitoff', 'hammer', 'lambert', 'mollweide', \ | ||
'polar', 'rectilinear'}, optional |
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.
Please leave as it was, the new form looks ugly at the terminal / pydoc.
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.
How about projection : string
and then specifying the list of allowed strings underneath?
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.
Sounds like a better idea I think.
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.
Specifying the list of allowed strings underneath is reasonable for long lists. I'm still not sure how you want it. Does ugly refer to changed style [ -> {
or the indentation. Reverted for now.
lib/matplotlib/figure.py
Outdated
------ | ||
axes : Axes | ||
------- | ||
axes : :class:`~.axes.Axes` |
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.
Types don't need to be marked up (just as you'd write foo : int
). Please leave as it was.
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.
Sometimes sphinx doesn't automatically put in links with out the markup though.
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.
Suite, I can always strip out unnecessary markup later...
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.
Reverted.
lib/matplotlib/figure.py
Outdated
@@ -1440,22 +1443,22 @@ def legend(self, *args, **kwargs): | |||
If shadow is activated and framealpha is ``None`` the | |||
default value is being ignored. | |||
|
|||
facecolor : None or "inherit" or a color spec | |||
facecolor : None or 'inherit' or a color spec |
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.
See #8079 re quote style (in fact I prefer double quotes...).
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.
Reverted.
lib/matplotlib/figure.py
Outdated
One of the file extensions supported by the active | ||
backend. Most backends support png, pdf, ps, eps and svg. | ||
|
||
*transparent*: | ||
transparent : {True, False} |
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.
bool
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.
To clarify, instead of saying {True, False}
it's easier to specify that the type has to be bool
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.
Changed. Thanks for the hint.
lib/matplotlib/figure.py
Outdated
mode : {"expand", None} | ||
If `mode` is set to ``"expand"`` the legend will be horizontally | ||
mode : {'expand', None} | ||
If `mode` is set to ``'expand'`` the legend will be horizontally |
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.
Please revert all changes up to here. I think what follows looks good.
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.
This one and similar ones reverted up to here. There were however some plain mistakes above, which lead to broken Sphinx rendering. I did not revert these.
Sorry, this is somewhat annoying. Writing documentation isn't the most fun part, but I thought I could give a little help here. But its no use if there are no clear rules. http://matplotlib.org/devdocs/devel/documenting_mpl.html#documenting-matplotlib |
Some of this was just "ported" 4 month ago ;-) |
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.
Hi @timhoffm thanks for putting together this PR; we're always keen to improve the documentation! Sorry about the confusion in places, our docstring style guides are a bit all over the place at the moment 😢
I've added a couple of suggestions for changes, overall this looks good though. Would it be okay to
- change
{True | False}
tobool
- where there are a list of strings, put them in the argument description
lib/matplotlib/figure.py
Outdated
One of the file extensions supported by the active | ||
backend. Most backends support png, pdf, ps, eps and svg. | ||
|
||
*transparent*: | ||
transparent : {True, False} |
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.
To clarify, instead of saying {True, False}
it's easier to specify that the type has to be bool
lib/matplotlib/figure.py
Outdated
['aitoff' | 'hammer' | 'lambert' | 'mollweide' | \ | ||
'polar' | 'rectilinear'], optional | ||
projection : {'aitoff', 'hammer', 'lambert', 'mollweide', \ | ||
'polar', 'rectilinear'}, optional |
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.
How about projection : string
and then specifying the list of allowed strings underneath?
lib/matplotlib/figure.py
Outdated
------ | ||
axes : Axes | ||
------- | ||
axes : :class:`~.axes.Axes` |
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.
Sometimes sphinx doesn't automatically put in links with out the markup though.
6673184
to
e231fd5
Compare
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.
Thanks a lot!
(Will merge if/when tests all pass) |
Thanks a lot for your contribution @timhoffm ! |
woo TYVM @timhoffm ! |
PR Summary
Fixes
Figure.add_subplot
,Figure.add_axes
from #7205 and some otherFigure
docstrings.