Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Draft
QuLogic wants to merge2 commits intomatplotlib:main
base:main
Choose a base branch
Loading
fromQuLogic:super-alias

Conversation

QuLogic
Copy link
Member

PR Summary

While working on#18189, I had to convert someself.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

  • Has pytest style unit tests (andpytest passes).
  • IsFlake 8 compliant (runflake8 on changed files to check).
  • [n/a] New features are documented, with examples if plot related.
  • [n/a] Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).
  • Conforms to Matplotlib style conventions (installflake8-docstrings andpydocstyle<4 and runflake8 --docstring-convention=all).
  • [n/a] New features have an entry indoc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented indoc/api/next_api_changes/ (follow instructions in README.rst there).

@anntzer
Copy link
Contributor

anntzer commentedSep 12, 2020
edited
Loading

At first glance, this is not really a problem with the aliasing machinery, right? Even if we wrote everything manually

classBase:defset_prop(self,val): ...# base class implementationdefset_alias(self,val):returnself.set_prop(val)classDerived:defset_prop(self,val): ...# derived class implementation

callingBase.set_alias(Derived()) would result in a call toDerived.set_prop, not a call toBase.set_prop. On the manual implementation side fixing this would basically require writing

classBase:defset_prop(self,val): ...# base class implementationdefset_alias(self,val):returnBase.set_prop(self,val)# explicitly use *this* classclassDerived:defset_prop(self,val): ...# derived class implementationdefset_alias(self,val):returnDerived.set_prop(self,val)# explicitly redeclare alias

When using the automatic machinery I could imagine making aliases notfunctions but custom descriptors with a__get__ that lets them know not only what instance they're called on but also what class, and then resolved the alias based on that, along the lines of (untested)

class_Alias:def__init__(self,real_name):self._real_name=real_namedef__get__(self,instance,owner):returngetattr(owner,self._real_name).__get__(instance,owner)

(plus whatever is needed to fake__doc__ and__name__ and whatnot) but do we really want to go there?

(Or did I completely miss something?)

@QuLogic
Copy link
MemberAuthor

At first glance, this is not really a problem with the aliasing machinery, right? Even if we wrote everything manually

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.

@anntzer
Copy link
Contributor

anntzer commentedSep 17, 2020
edited
Loading

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

@tacaswell
Copy link
Member

My understanding is that the core problem here is that when you dod.set_alias() you want to do late binding to get the derived class version ofset_prop, but when you doBase.set_alias you want to have done "early" binding and get the Base class version ofset_prop (I tried the "simple" fix of closing over the Base classes's target at class definition time and that just breaks the tests differently as thend.set_alias() falls back to the base classes version 🤦 ).

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

@anntzer
Copy link
Contributor

I'll let you take over my patch, then :)

@tacaswell
Copy link
Member

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

@github-actions
Copy link

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.

@github-actionsgithub-actionsbot added the status: inactiveMarked by the “Stale” Github Action labelAug 21, 2023
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
API: consistencystatus: inactiveMarked by the “Stale” Github Actionstatus: needs rebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@QuLogic@anntzer@tacaswell

[8]ページ先頭

©2009-2025 Movatter.jp