Alternative handling for close fixes #9770
Conversation
mikeshardmind
force-pushed
the
closing-fixes2
branch
from
b19b807
to
b153c40
Compare
discord/client.py
Outdated
| def __init_subclass__(cls): | ||
| """ | ||
| This exists to ensure we retain internal wrapping around close such | ||
| that it only runs once when called directly | ||
| """ | ||
| if cls.close is not Client.close: | ||
| # This is a bound method in __init_subclass__ | ||
| # There's no way to type this properly currently, | ||
| cls._user_close_method = cls.close # type: ignore | ||
| cls.close = Client.close |
There was a problem hiding this comment.
Why not just do this at __init__ instead?
There was a problem hiding this comment.
Doing it here, _user_close_method ends up a bound method (note: the comment is wrong there, sorry, that should state that it results in it being a bound method, and how, I'll update the PR with that in a moment.) and the reassignment only happens at subclass definition. In __init__, it would be unbound methods and require additional wrapping with types.MethodType or passing an instance where used.
For context, this is how I would go about it if you want it done in __init__:
def __init__(self, ...):
selftype = type(self)
if selftype.close is not Client.close:
self._user_close_method = types.MethodType(selftype.close, self)
self.close = types.MethodType(Client.close, self)There are a few reasons I'd prefer the way with __init_subclass__
__init_subclass__ is documented for this kind of modification in the python data model.
By contrast, types.MethodType is documented under a header that states:
If you instantiate any of these types, note that signatures may vary between Python versions.
I don't think the signature of it ever has been different than it is, but it's specifically left open to change, and the faster-cpython stuff was looking at method and bound method lookup costs as a place to improve performance, so I wouldn't be surprised if it does in the future.
It ends up playing nicer with the language server in VSC this way, not sure about other language servers, and should play nicer with typing after stabilization of some in-progress specification work.
It ends up putting the only part of __init__ / __subclass_init__ that's wrapping around something a user did near what relies on it in the code. It's minor, but I find that locating things that are related nearby when reasonable helps when reading code that does something strange.
Not a reason here:
The performance costs of doing this in __init__ instead are currently negligible even in a case where this would be called somewhat repeatedly across multiple instances. In the case expected here, it should remain negligible no matter what happens with faster-cpython work and is not a major reason to prefer one over the other.
|
This doesn't work as-is and would need other changes which would make it more complex, closing for now, can follow up in discord later. |
Summary
See #9769 And discussion https://discord.com/channels/336642139381301249/1222571653272309842/1222915657046167687
Presented for comparison and testability of options.
Checklist