Require calling a _BoundMethodProxy to get the underlying callable. #9084
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
bef9d16 to
28fd2d7
Compare
|
rewritten to use weakref.WeakMethod directly edit: needs some more work. |
28fd2d7 to
fb6b895
Compare
|
rebased |
7e682ba to
ad09280
Compare
tacaswell
reviewed
May 7, 2018
| # Note proxy not needed in python 3. | ||
| # TODO rewrite this when support for python2.x gets dropped. | ||
| proxy = _BoundMethodProxy(func) | ||
| self._func_cid_map.setdefault(s, {}) |
There was a problem hiding this comment.
pros / cons of using ismthod vs try:....except:... on attempting to create the WeakMethod?
There was a problem hiding this comment.
Edited accordingly.
As mentioned elsewhere I believe quite strongly that the use of weakmethod semantics is pretty silly anyways...
ad09280 to
04ebedc
Compare
04ebedc to
85e07fb
Compare
|
Circle builds the commit on the branch (rather than the commit from doing the merge) so those won't fix them selves. |
QuLogic
reviewed
Jul 10, 2018
| @@ -0,0 +1,6 @@ | |||
| `CallbackRegistry` now stores callbacks using stdlib's `WeakMethods` | |||
| ```````````````````````````````````````````````````````````````````` | ||
|
|
||
| In particular, this implies that ``CallbackRegistry.callbacks[signal]`` is now | ||
| a mapping of callback ids to `WeakMethods` (i.e., they need to be first called |
Code that used to call `_proxy(...)` now needs to call `_proxy()(...)`. Instead of catching a ReferenceError, that could be either raised by a failure to dereference the proxy, or by the underlying callable, one can now check that `_proxy()` does not return None (to distinguish between the two cases). This is the same design as the stdlib's WeakMethod.
85e07fb to
5257c4f
Compare
tacaswell
reviewed
Jul 20, 2018
| return self._hash | ||
|
|
||
|
|
||
| def _exception_printer(exc): |
There was a problem hiding this comment.
Why don't we need this anymore?
There was a problem hiding this comment.
It's still there (line 65) and unchanged, the diff is just confused.
There was a problem hiding this comment.
Ah, great sorry for the noise.
tacaswell
approved these changes
Jul 22, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Code that used to call
_proxy(...)now needs to call_proxy()(...).Instead of catching a ReferenceError, that could be either raised by a
failure to dereference the proxy, or by the underlying callable, one can
now check that
_proxy()does not return None (to distinguish betweenthe two cases).
This is the same design as the stdlib's WeakMethod.
See #9063 (comment).
PR Summary
PR Checklist