Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Use function signatures in boilerplate.py.#10918
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
lib/matplotlib/pyplot.py Outdated
| ax = gca() | ||
| ret = ax.acorr(x, data=data, **kwargs) | ||
| def acorr( | ||
| x, data=None, **kwargs |
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.
Is there a good reason to have all these newlines? I think they make the code more unreadable.
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.
Got rid of the ones after the argument lists. The ones before the argument list are present to make code generation simpler (we can just always indent by 8); note that previously the calls had sometimes "weird" indentations.
e90e2ea to7f3014dCompareCan we make data kwarg only in the generated functions? |
| } | ||
| def format_value(value): | ||
| class value_formatter: |
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.
Why make this a class?
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.
because str(parameter) calls repr(default), so that's how we override its output?
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.
ah, understand now.
I don't understand your comment about the data kwarg?... |
Formatting it something like |
7f3014d to4d19279Compare4d19279 to86fe16bComparedone As a side note the improved signature of text() and annotate() was apparently because I was running this from a venv with mpl master, whereas last release had less informative signatures. (Unless doing some "interesting" tricks) we can't actually run boilerplate.py as part of setup.by build (as discussed earlier) because it relies on being able to import axes.py to get the signatures... |
lib/matplotlib/pyplot.py Outdated
| return ret | ||
| @docstring.copy_dedent(Axes.acorr) | ||
| def acorr( |
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.
Is there a reason to put all the arguments on a new line. This looks rather unusual.
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.
If it's just because of the text wrapping, you could passinitial_indent=' ' * (len(func) + 5) totext_wrapper.fill(). Thenlstrip() or slice to[len(func)+5:] so that the first line has an appropriate length to fit afterdef thefunc(.
Even simpler, passinitial_indent='def thefunc(' totext_wrapper.fill().
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.
It makes the autogeneration quite a bit simpler (specifically to properly wrap the first line -- the linewrapping code actually doesn't know ).
I would argue that while we want "reasonable" style for this autogenerated code, we shouldn't overthink it either...
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.
Then you end up with
def some_long_function(some_long_arg_list, ... some_more_args, etc)which is not optimal either (you'd either want to align some_more_args under some_long_arg_list, or have a newline after the parenthesis).
The first solution is... honestly not worth it IMO.
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.
That's actually what I wanted. But not anymore after rechecking PEP-0008. 😄
I'm fine with all args on new line for longer signatures. For short ones see my other comment.
tools/boilerplate.py Outdated
| param.replace(default=value_formatter(param.default)) | ||
| if param.default is not param.empty else param | ||
| for param in params])) | ||
| # Move opening parenthesis before newline. |
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.
Only wrap for longer signatures by making this conditional:
if MAX_PREFIX + len(func) + len(sig) >= 80:
MAX_PREFIX is probably 18 (for __ret = gca().).
Sorry for being a bit picky, but users will see these if they look at the source, e.g. via?? in ipython or if we plan to link the source in the docs in the future.
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.
feel free to push the change to my branch
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.
Honestly I think that when the source looks like
In [1]: plt.errorbar??Signature: plt.errorbar(x, y, yerr=None, xerr=None, fmt='', ecolor=None, elinewidth=None, capsize=None, barsabove=False, lolims=False, uplims=False, xlolims=False, xuplims=False, errorevery=1, capthick=None, *, data=None, **kwargs)Source: @docstring.copy_dedent(Axes.errorbar)def errorbar( x, y, yerr=None, xerr=None, fmt='', ecolor=None, elinewidth=None, capsize=None, barsabove=False, lolims=False, uplims=False, xlolims=False, xuplims=False, errorevery=1, capthick=None, *, data=None, **kwargs): return gca().errorbar( x=x, y=y, yerr=yerr, xerr=xerr, fmt=fmt, ecolor=ecolor, elinewidth=elinewidth, capsize=capsize, barsabove=barsabove, lolims=lolims, uplims=uplims, xlolims=xlolims, xuplims=xuplims, errorevery=errorevery, capthick=capthick, data=data, **kwargs)File: ~/src/extern/matplotlib/lib/matplotlib/pyplot.pyType: functionthe biggest issue is not with an extra newline, but that that's a useless source...
We could set__wrapper__ to the inner function so that getsource points toAxes.errorbar, but that may be confusing for functions that do call sci() with the return value...
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.
No doubt not quite useful, but technically and style-wise correct. That's ok by me. I wouldn't set__wrapper__.
My point was more towards the short ones like
def table( **kwargs): return gca().table( **kwargs)which should be (and is now with my push to your branch)
def table(**kwargs): return gca().table(**kwargs)
timhoffm left a comment• 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.
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.
Conditionally on CI pass.
I won't merge myself because I've contributed the last change to the PR. And that should be reviewed/approved by someone else.
tools/boilerplate.py Outdated
| param.replace(default=value_formatter(param.default)) | ||
| if param.default is not param.empty else param | ||
| for param in params])) | ||
| if len('def ') + len(func) + len(sig) >= 80: |
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.
len('def ' + func + sig)
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 merge after this (effectively there's been two review approvals) (or even without fixing it if you don't want to).
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 thought I won't construct a new string. But you're right. This is unneccesary micro-optimization.
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, will merge after CI pass, or do it yourself if you are around more early than me.
2d5b934 to0293e8aCompareSelf-merging based on#10918 (review) and#10918 (comment). |
It turns out that passing named arguments for everything broke Cartopy a bit because the first parameter of |
It's also started explicitly passing |
anntzer commentedSep 25, 2018 • 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.
Sorry about that. It doesn't look too hard to make pyplot pass defaultless arguments positionally instead of as keywords if you'd like. |
QuLogic commentedSep 25, 2018 • 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.
We probably want to keep compatibility with old Cartopy, so we should change the positional arguments at least. As to |
Uh oh!
There was an error while loading.Please reload this page.
PR Summary
Proposed followup to#10485. As it turns out we "need" to keep storing the return value of axes methods because we sometimes pass it to
sci.Also, using a simpler algorithm actually allows getting better signatures for a few functions (annotate and quiverkey).
PR Checklist