Remove __dict__ from __slots__
#2619
Conversation
|
py 3.6 related tests fail - as expected. |
|
@harshil21 You can probably remove them and push that directly to the branch, no need to run unneeded test suits |
# Conflicts: # tests/test_slots.py
Bibo-Joshi
left a comment
Bibo-Joshi
left a comment
There was a problem hiding this comment.
base.pyhas_id_attrsas class variable. Should we consider setting this attribute in__new__? If done, it will also print the correct info when (some_tg_object_class_instance.__class__._id_attrs) is printed.
I don't understand the second part. Isn't some_tg_object_class_instance.__class__._id_attrs a class attribute as it's now?
Anyway, I agree that it would be cleaner to have _id_attr as instance attribute. We can either go with __new__ or give TelegramObject an __init__ and call it in all the subclasses. I feel like the second one would be somewhat cleaner, as AFAIK __new__ isn't really there for initialization. In fact with __new__, we should double check if it makes any trouble with pickle 🤔 And calling super().__init__() is mostly a one time effort to add … OTOH I see that we already did this for filters … What do you think?
apart from this, I only have a few minor remarks (see below). Obviously I didn't check all the tests :D
Okay nvm that, I wanted to dig deeper into that cause pycharm now apparently flags that as a warning (saying
No? In fact
I guess we don't really gain or lose anything out from doing this so I think its probably better to leave it as it is |
yes, filters uses
with |
|
Okay, moved the Edit: A better workaround is described in 88a7e13 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Bibo-Joshi
added
🔌 enhancement
Notes about the changes (for further discussion + ease of review):
BasePersistenceneeds__dict__(because of the replacement of methods we do in__new__),CallbackContextby design has__dict__, should any other classes have it too?base.pyhas_id_attrsas class variable. Should we consider setting this attribute in__new__? If done, we can reduce some code duplication and fix the pycharm warning. Edit: Done._id_keysindocument.py, apparently its never been used.Notes about tests:
test_slots.pynow checks that each class doesn't have a__dict__.Dispatcherin tests so we can add custom attributes, but a test failedtest_dispatcher.py/test_multiple_async_decorator- didn't raise the error, so dispatcher has__dict__.tests/test_inlinequeryhandler.py/test_chat_types)test_updater/test_webhook,test_conversationhandler.py/test_schedule_job_exceptionto useDictBotandDictJobQueueto monkeypatch on it.Also I try to group similar changes into one commit, so review by commit than viewing everything at once