bpo-36669: Add matmul support for weakref.proxy by mdickinson · Pull Request #12932 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation9 Commits5 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
weakref.proxy objects currently support all numeric methods except the matrix multiplication operators (@ and @=). This PR adds that missing support.
https://bugs.python.org/issue36669
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty straightforward. I assume __rmatmul__ is covered automatically by implementing __matmul__, but I didn't check. Either way I think adding a test for that would be good.
Also, it seems there is no pure python implementation of weakref.proxy? I checked Modules/weakref.py and didn't see one, so I assume that this is fine, but maybe I'm missing it. Just thinking of PEP 399.
| @@ -285,6 +285,18 @@ def __ifloordiv__(self, other): |
|---|
| p //= 5 |
| self.assertEqual(p, 21) |
| def test_proxy_matmul(self): |
| class C: |
| def __matmul__(self, other): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a test for __rmatmul__ as well?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good plan. Done!
@pganssle Thanks for reviewing! Re Python-only implementations, I think discussion of PEP 399-ifying of the weakref module is probably out of scope for this PR; maybe we could open a new issue for that?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks for reviewing! Re Python-only implementations, I think discussion of PEP 399-ifying of the weakref module is probably out of scope for this PR; maybe we could open a new issue for that?
Yes, I fully agree that it's out of scope. I only mentioned it because it seems like it's maybe partially compliant with PEP 399, so I wasn't sure if I was just missing a pure python implementation of weakref.proxy that would need to be updated. If one does not exist (and it seems it doesn't), then I think that's a different issue to be solved.
One other question: Does this need a "What's New" entry, or a .. versionchanged:: note in the documentation?
I think it's on the borderline in terms of changes that need to be in What's New / versionchanged. On the one hand, it seems like it was an oversight that it wasn't included, so it seems odd to call it out specifically as a feature addition. On the other hand, it may frustrate some people if the only place that they can find that documents when weakref.proxy got matmul support is in the "news" changelog.
Does this need a "What's New" entry, or a
.. versionchanged::note in the documentation?
Yeah, I thought about this and couldn't decide. :-) I agree with your "borderline" comment. I'll go ahead and add a "whatsnew" entry; worst case is that whoever tidies up the 3.8 "what's new" deletes that entry as insignificant.
I also couldn't decide whether to add the Misc/NEWS entry under the "Library" section or the "Core and Builtins" section: we're actually modifying a built-in Python object (Objects/weakrefobject.c) that's then exposed to Lib/weakref.py via an extension module Modules/_weakref.c, so the code lives in three separate places. So "Core and Builtins" seems technically more accurate, but "Library" better reflects how users see it. I'll stick with "Library" unless anyone objects violently.
I'll go ahead and add a "whatsnew" entry
... and the .. versionchanged entry, too.