Until now, all alias methods had the signature*args, **kwargs. This is not very helpful as it's unclear which arguments are expected (well, usually none for getters and one for setters, but the alias method does not tell).

This PR uses inspect to copy the signatures of the original functions to the alias.

Example setter

before:

now:

Example getter

before:

now:

timhoffm added this to thev3.1 milestone

Aug 26, 2018

timhoffm added the API: consistency label

Aug 26, 2018

anntzer reviewed

Aug 26, 2018

View reviewed changes

lib/matplotlib/cbook/__init__.py Outdated

		@@ -1914,20 +1914,28 @@ class so far, an alias named ``get_alias`` will be defined; the same will
		if cls is None: # Return the actual class decorator.
		return functools.partial(_define_aliases, alias_d)

		def make_alias(name): # Enforce a closure over name.
		def method(self, args, *kwargs):

Copy link

Contributor

anntzerAug 26, 2018

Choose a reason for hiding this comment

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

Just add a@functools.wraps(getattr(cls, name)) decorator here? inspect.signature will then get the signature from the wrappee. Note that you can even play with theassigned kwarg to functools.wraps if you don't want to update the name...

Copy link

MemberAuthor

timhoffmAug 26, 2018

Choose a reason for hiding this comment

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

Thanks for the hint. That would work too.

The getattr-alias is a bit slower than my explictly coded lambda (67ns vs. 26ns). I mean, it's not much in absolute values, but the aliases might be used a lot by users. Do we want the added speedup or should we stay with the slightly simpler but slower getattr implementiation.

Copy link

Contributor

anntzerAug 26, 2018•
edited
Loading

Choose a reason for hiding this comment

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

No strong opinion there; though eval() whill likely have a small extra startup cost as well?

Copy link

MemberAuthor

timhoffmAug 26, 2018

Choose a reason for hiding this comment

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

There are 60 properties. The package import time does not measureably change between the two approaches (200.8 +/- 6.0ms vs. 201.0 +/- 6.0ms).

Probably both approaches are fast enough in startup as well as runtime use to not make a difference. I will go back to decorating the original function to keep it simpler. If runtime performance is really super-critical, users should just use the methods functions, not the aliases.

anntzer reviewed

Aug 26, 2018

View reviewed changes

lib/matplotlib/cbook/__init__.py Outdated

		return method
		def make_alias(method):
		import inspect
		args = str(inspect.signature(method))[1:-1] # remove parentheses

Copy link

Contributor

anntzerAug 26, 2018

Choose a reason for hiding this comment

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

str(inspect.signature(functools.partial(method, None))) will bind the first argument for you and remove it from the signature more easily than the text wrangling below :) (plus it even works if someone decides to not call the first argument "self", hehe... for example it could just be a wrapper function itself being*args, **kwargs)

Copy link

MemberAuthor

timhoffm commentedAug 26, 2018

For future reference, this was the first approach, which was discarded in favor of simply decorating the alias with@functools.wraps(getattr(cls, name)).

    def make_alias(method):        import inspect        args = str(inspect.signature(method))[1:-1]  # remove parentheses        if args[:6] == 'self, ':            call_args = args[6:]        elif args == 'self':            call_args = ''        else:            raise RuntimeError('Unexpected signature: %s' % args)        alias_method = eval('lambda {}: self.{}({})'.format(            args, method.__name__, call_args))        alias_method.__doc__ = "Alias for `{}`.".format(method.__name__)        return alias_method    for prop, aliases in alias_d.items():        exists = False        for prefix in ["get_", "set_"]:            if prefix + prop in vars(cls):                exists = True                for alias in aliases:                    method = make_alias(getattr(cls, prefix + prop))                    method.__name__ = prefix + alias                    setattr(cls, prefix + alias, method)        if not exists:            raise ValueError(                "Neither getter nor setter exists for {!r}".format(prop))