Skip to content

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 1, 2018

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Mar 30, 2018

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

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@anntzer anntzer added this to the v3.0 milestone Mar 30, 2018
ax = gca()
ret = ax.acorr(x, data=data, **kwargs)
def acorr(
x, data=None, **kwargs
Copy link
Member

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.

Copy link
Contributor Author

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.

@anntzer anntzer force-pushed the signature-boilerplate branch 4 times, most recently from e90e2ea to 7f3014d Compare March 30, 2018 10:22
@tacaswell
Copy link
Member

Can we make data kwarg only in the generated functions?

}

def format_value(value):
class value_formatter:
Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Member

Choose a reason for hiding this comment

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

ah, understand now.

@anntzer
Copy link
Contributor Author

anntzer commented Mar 31, 2018

I don't understand your comment about the data kwarg?...

@tacaswell
Copy link
Member

Formatting it something like def func(x, y, *, data=None) so data does not become positional.

@anntzer anntzer force-pushed the signature-boilerplate branch from 7f3014d to 4d19279 Compare March 31, 2018 20:13
@anntzer anntzer force-pushed the signature-boilerplate branch from 4d19279 to 86fe16b Compare March 31, 2018 20:30
@anntzer
Copy link
Contributor Author

anntzer commented Mar 31, 2018

done

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


return ret
@docstring.copy_dedent(Axes.acorr)
def acorr(
Copy link
Member

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.

Copy link
Member

@timhoffm timhoffm Mar 31, 2018

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 pass initial_indent=' ' * (len(func) + 5) to text_wrapper.fill(). Then lstrip() or slice to [len(func)+5:] so that the first line has an appropriate length to fit after def thefunc(.
Even simpler, pass initial_indent='def thefunc(' to text_wrapper.fill().

Copy link
Contributor Author

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

Copy link
Contributor Author

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.

Copy link
Member

@timhoffm timhoffm Mar 31, 2018

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.

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.
Copy link
Member

@timhoffm timhoffm Mar 31, 2018

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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.py
Type:      function

the 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 to Axes.errorbar, but that may be confusing for functions that do call sci() with the return value...

Copy link
Member

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)

Copy link
Member

@timhoffm timhoffm left a 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.

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

Choose a reason for hiding this comment

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

len('def ' + func + sig)

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Member

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.

@timhoffm timhoffm force-pushed the signature-boilerplate branch 2 times, most recently from 2d5b934 to 0293e8a Compare April 1, 2018 01:40
@anntzer
Copy link
Contributor Author

anntzer commented Apr 1, 2018

Self-merging based on #10918 (review) and #10918 (comment).

@anntzer anntzer merged commit 5a8f4ea into matplotlib:master Apr 1, 2018
@anntzer anntzer deleted the signature-boilerplate branch April 1, 2018 02:10
@QuLogic
Copy link
Member

QuLogic commented Sep 25, 2018

It turns out that passing named arguments for everything broke Cartopy a bit because the first parameter of imshow there is img, not X.

@QuLogic
Copy link
Member

QuLogic commented Sep 25, 2018

It's also started explicitly passing data=None to all wrapped Axes methods that accept labelled data, which is slightly problematic since _preprocess_data is not public.

@anntzer
Copy link
Contributor Author

anntzer commented Sep 25, 2018

Sorry about that. It doesn't look too hard to make pyplot pass defaultless arguments positionally instead of as keywords if you'd like.
I guess you prefer not passing data through when it's None? (There's a plan to make _preprocess_data public too but that's a side point.) Although they were already passing it before (at least in some cases, didn't check carefully) so I'm not sure what changed?

@QuLogic
Copy link
Member

QuLogic commented Sep 25, 2018

We probably want to keep compatibility with old Cartopy, so we should change the positional arguments at least.

As to data, it was never explicit before, but caught by **kwargs. If never passed, it would not appear at all in the final call. Now it's there explicitly (even if None). I'm not sure if it should or should not be explicit in the signature, but it would be best if it's not passed when None at the very least.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants