-
-
Notifications
You must be signed in to change notification settings - Fork 18.2k
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: document optional arguments on plot submethods #27381
DOC: document optional arguments on plot submethods #27381
Conversation
Is there an issue for this? |
|
2a80564
to
a0bdf0b
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.
Rather than copy / paste this can you use the Appender decorator that we use for other docstrings? You can see a use of this in pandas.core.generic amongst other places
@hancar can you fix up merge conflicts and address comment above? |
Oh, thank you for reminding, today I will find some time. |
a0bdf0b
to
78c9620
Compare
Hello @hancar! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2019-11-19 08:22:52 UTC |
78c9620
to
83051d5
Compare
Seems like some test and code checks are failing. Let us know if you need help looking into them. |
34adc6c
to
f0c41a0
Compare
f0c41a0
to
6d85054
Compare
9a2f226
to
d1a843b
Compare
Yes please, now I would really appreciate some help. It seems like the tests fail on some linting or docstring validation, but I didn't manage to rerun the tests locally to find the problem. This is from
Can you give me some hint, please? |
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.
doesn't look like CI logs are available any more so not sure on error; you can of course run scripts/validate_docstrings.py locally which will likely give you more info
pandas/plotting/_core.py
Outdated
@@ -588,7 +607,7 @@ class PlotAccessor(PandasObject): | |||
labels with "(right)" in the legend | |||
include_bool : bool, default is False | |||
If True, boolean values can be plotted. | |||
**kwargs | |||
`**kwargs` : keywords |
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.
Shouldn't need the backticks here
d1a843b
to
50bba72
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.
I think this is a nice PR. Let's see if we can fix up a bit and get this in
pandas/plotting/_core.py
Outdated
@@ -588,7 +607,7 @@ class PlotAccessor(PandasObject): | |||
labels with "(right)" in the legend | |||
include_bool : bool, default is False | |||
If True, boolean values can be plotted. | |||
**kwargs | |||
**kwargs : keywords |
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.
I think you can revert this; shouldn't be necessary for args / kwargs
pandas/plotting/_core.py
Outdated
@@ -1051,7 +1084,8 @@ def box(self, by=None, **kwargs): | |||
---------- | |||
by : str or sequence | |||
Column in the DataFrame to group by. | |||
**kwargs | |||
%s | |||
**kwargs : 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.
same comment on kwargs
|
||
def bar(self, x=None, y=None, **kwargs): | ||
""" | ||
@Appender(_line_docs % _shared_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.
Rather than putting this all in Appender you can use Appender with Substitution decorators. Check pandas.core.frame for pre-existing examples
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.
Just to be sure: do you mean to replace
@Appender(_line_docs % _shared_docs)
by
@Substitution(_shared_docs)
@Appender(_line_docs)
???
For me this is just more decorators making the code more complex, but not really nicer. Even in pandas.core.frame there are counter examples:
@Appender(_shared_docs["align"] % _shared_doc_kwargs)
So, do we really want that many decorators?
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.
OK sure. Looks like some CI failures can you merge master and we can get this one in
pandas/plotting/_core.py
Outdated
def bar(self, x=None, y=None, **kwargs): | ||
""" | ||
@Appender(_line_docs % _shared_docs) | ||
def line(self, x=None, y=None, figsize=None, subplots=False, sharex=None, |
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.
While doing this you might as well make all of the new keywords "keyword only", i.e. don't allow them to be called positionally. Easier to do now than to try and go back and do it later
50bba72
to
002fc51
Compare
@hancar looks like some CI issues can you take a look? |
Closing as stale, @hancar if you want to finish this, please let us know to reopen. |
black pandas