-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Simplifying the implementation of signature-overloaded functions #7966
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
Comments
I see the attraction to something like this, but a contrary view is that it is adding a fair-size chunk of extraordinarily complex, hard-to-understand code (your decorator machinery) in order to replace existing code that mostly works fine with a similar amount of code. So it looks to me like it adds complexity and LOC for no real gain. |
Actually, I kind of like this idea. One of the things that has driven me nuts with the "chameleon functions" is that the argument mucking is often times intertwined with the function logic. This helps to separate it out. And the ability to easily perform deprecations on specific call signatures is also valuable. Now, the big questions is... is pycharms going to hate us even more? |
This might make pycharm hate us less that |
As a side note, a possibly simpler implementation (but without support for kwonly args on Py2 (other than manually, as usual)) was proposed in #6404 (comment). |
This is basically now in as diff --git i/lib/matplotlib/quiver.py w/lib/matplotlib/quiver.py
index c6e51afc0a..23e64b4194 100644
--- i/lib/matplotlib/quiver.py
+++ w/lib/matplotlib/quiver.py
@@ -404,21 +404,17 @@ def _parse_args(*args, caller_name='function'):
caller_name : str
Name of the calling method (used in error messages).
"""
- X = Y = C = None
-
- nargs = len(args)
- if nargs == 2:
- # The use of atleast_1d allows for handling scalar arguments while also
- # keeping masked arrays
- U, V = np.atleast_1d(*args)
- elif nargs == 3:
- U, V, C = np.atleast_1d(*args)
- elif nargs == 4:
- X, Y, U, V = np.atleast_1d(*args)
- elif nargs == 5:
- X, Y, U, V, C = np.atleast_1d(*args)
- else:
- raise _api.nargs_error(caller_name, takes="from 2 to 5", given=nargs)
+ args = [*map(np.atleast_1d, args)] # handle both scalars and masked arrays
+ try:
+ X, Y, U, V, C = _api.select_matching_signature([
+ lambda U, V: (None, None, U, V, None),
+ lambda U, V, C: (None, None, U, V, C),
+ lambda X, Y, U, V: (X, Y, U, V, None),
+ lambda X, Y, U, V, C: (X, Y, U, V, C),
+ ], *args)
+ except TypeError:
+ raise _api.nargs_error(
+ caller_name, takes="from 2 to 5", given=len(args)) from None
nr, nc = (1, U.shape[0]) if U.ndim == 1 else U.shape
but let's consider this only for cases where the helper is really needed (which correspond to the current places where select_matching_signature is indeed used). |
There are many functions in matplotlib that have "interesting" call signatures, e.g. that could be called with 1, 2 or 3 arguments with different semantics (i.e. not just binding arguments in order and having defaults for the later ones). In this case, the binding of arguments is typically written on an ad-hoc basis, with some bugs (e.g. the one I fixed in https://github.com/matplotlib/matplotlib/pull/7859/files#diff-84224cb1c8cd1f13b7adc5930ee2fc8fR365) or difficult to read code (e.g. https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/quiver.py#L374 (
quiver._parse_args
)).Moreover, there are also cases where a function argument should be renamed, but cannot be due to backwards compatibility considerations (e.g. #7954).
I propose to fix both issues using a signature-overloading decorator (see below for prototype implementation). Basically, the idea would be to write something like
where the first overload that can bind the arguments is the one used.
In order to support changes in signature due to argument renaming, an overload with the previous signature that raises a DeprecationWarning before forwarding the argument to the "inner" function can be used.
Thoughts?
Protoype implementation:
The text was updated successfully, but these errors were encountered: