-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Property aliases don't work right with super #18462
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
base: main
Are you sure you want to change the base?
Conversation
61ff8b4
to
43a4cf1
Compare
At first glance, this is not really a problem with the aliasing machinery, right? Even if we wrote everything manually class Base:
def set_prop(self, val): ... # base class implementation
def set_alias(self, val): return self.set_prop(val)
class Derived:
def set_prop(self, val): ... # derived class implementation calling class Base:
def set_prop(self, val): ... # base class implementation
def set_alias(self, val): return Base.set_prop(self, val) # explicitly use *this* class
class Derived:
def set_prop(self, val): ... # derived class implementation
def set_alias(self, val): return Derived.set_prop(self, val) # explicitly redeclare alias When using the automatic machinery I could imagine making aliases not functions but custom descriptors with a class _Alias:
def __init__(self, real_name):
self._real_name = real_name
def __get__(self, instance, owner):
return getattr(owner, self._real_name).__get__(instance, owner) (plus whatever is needed to fake (Or did I completely miss something?) |
The difference is that aliases are supposed to be (at least to me) transparently equivalent. They should work exactly the same as the main version. The user has no idea they're calling an alias, but if the alias is called for the middle of the class hierarchy, it should not magically call the real version at the end of the hierarchy. |
I don't have a really strong opinion there. If we do want to fix this, do you agree that the custom descriptor is the way to go, or can you think of something simpler? With the descriptor approach, the following patch fixes the tests AFAICT, feel free to adopt it: diff --git i/lib/matplotlib/artist.py w/lib/matplotlib/artist.py
index 274a35ea0..6d375c73d 100644
--- i/lib/matplotlib/artist.py
+++ w/lib/matplotlib/artist.py
@@ -1305,7 +1305,7 @@ class ArtistInspector:
and callable(getattr(self.o, name))]
aliases = {}
for name in names:
- func = getattr(self.o, name)
+ func = inspect.getattr_static(self.o, name)
if not self.is_alias(func):
continue
propname = re.search("`({}.*)`".format(name[:4]), # get_.*/set_.*
diff --git i/lib/matplotlib/cbook/__init__.py w/lib/matplotlib/cbook/__init__.py
index 2b52dd414..04e6b0661 100644
--- i/lib/matplotlib/cbook/__init__.py
+++ w/lib/matplotlib/cbook/__init__.py
@@ -1859,6 +1859,15 @@ def _str_lower_equal(obj, s):
return isinstance(obj, str) and obj.lower() == s
+class _Alias:
+ def __init__(self, real_name):
+ self.__name__ = real_name
+ self.__doc__ = f"Alias for `{real_name}`."
+
+ def __get__(self, instance, owner):
+ return getattr(owner, self.__name__).__get__(instance, owner)
+
+
def _define_aliases(alias_d, cls=None):
"""
Class decorator for defining property aliases.
@@ -1880,22 +1889,13 @@ def _define_aliases(alias_d, cls=None):
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*.
- @functools.wraps(getattr(cls, name))
- def method(self, *args, **kwargs):
- return getattr(self, name)(*args, **kwargs)
- return 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(prefix + prop)
- method.__name__ = prefix + alias
- method.__doc__ = "Alias for `{}`.".format(prefix + prop)
- setattr(cls, prefix + alias, method)
+ setattr(cls, prefix + alias, _Alias(prefix + prop))
if not exists:
raise ValueError(
"Neither getter nor setter exists for {!r}".format(prop)) |
My understanding is that the core problem here is that when you do I agree that if we want to make this work the descriptor approach is the only option (as we need to know the context in which the method has been accessed). |
I'll let you take over my patch, then :) |
Even slowly convincing my self this is a good idea as the other alternatives are a) leave in broken in this way (which is a surprising an subtle kind of broken) b) deprecating all of the aliases. Neither of which is great.... |
Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it. |
PR Summary
While working on #18189, I had to convert some
self.set_bar(...)
tosuper().set_bar(...)
, but it turned out thatbar
was an alias forfoo
, and it kept callingObject3D.set_foo
instead ofObject.set_foo
. This was easy enough to convert to the original non-alias property setters, but maybe this should work?I haven't quite figured out how to actually fix it though, so consider this a bug report in the form of a broken test (
test_define_aliases_subclass
). cc @anntzer I think wrote this originally.PR Checklist
pytest
passes).flake8
on changed files to check).flake8-docstrings
andpydocstyle<4
and runflake8 --docstring-convention=all
).doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).