Member

Notes about the changes (for further discussion + ease of review):

• BasePersistence needs __dict__ (because of the replacement of methods we do in __new__), CallbackContext by design has __dict__, should any other classes have it too?
• base.py has _id_attrs as 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.
• drops py3.6 related slots code, since we're dropping py3.6 support anyway.
• removed _id_keys in document.py, apparently its never been used.

Notes about tests:

• removed missing slot + warning about custom attribute tests - since python/pre-commit will now raise errors for both cases.
• test_slots.py now checks that each class doesn't have a __dict__.
• subclassed Dispatcher in tests so we can add custom attributes, but a test failed test_dispatcher.py/test_multiple_async_decorator - didn't raise the error, so dispatcher has __dict__.
• fixed a test (tests/test_inlinequeryhandler.py/test_chat_types)
• modified test_updater/test_webhook, test_conversationhandler.py/test_schedule_job_exception to use DictBot and DictJobQueue to monkeypatch on it.

Also I try to group similar changes into one commit, so review by commit than viewing everything at once

Member Author

py 3.6 related tests fail - as expected.

Member

@harshil21 You can probably remove them and push that directly to the branch, no need to run unneeded test suits

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

base.py has _id_attrs as 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

Member Author

I don't understand the second part. Isn't some_tg_object_class_instance.__class__._id_attrs a class attribute as it's now?

Okay nvm that, I wanted to dig deeper into that cause pycharm now apparently flags that as a warning (saying _id_attrs is read-only and that it has no attribute _id_attrs which doesn't make sense)

OTOH I see that we already did this for filters …

No? In fact BaseFilter uses __new__ to assign instance attributes, like what we can do here.

What do you think?

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

Member

OTOH I see that we already did this for filters …

No? In fact BaseFilter uses __new__ to assign instance attributes, like what we can do here.

yes, filters uses __new__, that's what I meant :D

What do you think?

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

with __new__ we can drop _id_attrs from the slots of all the subclasses (by instead just declaring that slot for TelegramObject) and we make it a proper instance attribute, which has at least some code quality value IMO …

Member Author

Okay, moved the _id_attrs to __new__. Apparently type hints can't be applied on instances in __new__ so had to type hint _id_attrs in other classes (wherever mypy complained).

Edit: A better workaround is described in 88a7e13

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.