Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add __slots__ #2345

Merged
merged 36 commits into from
May 29, 2021
Merged

Add __slots__ #2345

merged 36 commits into from
May 29, 2021

Conversation

harshil21
Copy link
Member

@harshil21 harshil21 commented Jan 29, 2021

This PR adds __slots__ to all classes (finally!). As discussed in #1109, __dict__ is currently added to slots for backward compatibility.

Some notes about the changes:

  • __weakref__ was added in Dispatcher's __slots__ as we're creating weakrefs with it.
  • Reworked to_dict() in base.py so it works with slots
  • Also included CSI style comments wherever necessary.
  • Make _request an instance attribute in updater.py.
  • Private classes (not exposed for the users) don't have __dict__.
  • TelegramDeprecationWarning is raised when users try to assign new attributes on classes which don't support it.
  • See this comment for more changes.

Closes #1109

@harshil21
Copy link
Member Author

Looks like in python 3.6 ABC's __slots__ has a __dict__, so those tests are failing. Other test fails are unrelated. Will fix in next commit

Also tried to fix that py3.6 test fail
Fixed and added some slots
Update class attribute properly in updater.py
@harshil21 harshil21 marked this pull request as ready for review January 31, 2021 03:28
Copy link
Member

@Bibo-Joshi Bibo-Joshi left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the big PR! I left a bunch of comments, though I didn't check the slots for ever class and skimmed the test :D

Regarding tests: You currently test for extra slots in each class's test. I would like to see another test that iterates over all modules and makes every class actually has slots. Could go to a standalone test_slots.py. If testing for additional slots can work on class.__slots__ instead of class instances, one could also think about moving the tests for extra slots to that test file. Would save duplication and we wouldn't have to remember putting the test_extra_slots in each test file …

Tests that I would also like to see and which could go into test_slots.py:

  • Test that class.custom_attribute actually gives a warning
  • Test that overwriting existing attributes still works and doesn't give a warning

telegram/base.py Outdated Show resolved Hide resolved
telegram/base.py Outdated Show resolved Hide resolved
telegram/base.py Show resolved Hide resolved
telegram/chatmember.py Show resolved Hide resolved
telegram/base.py Show resolved Hide resolved
telegram/utils/deprecate.py Show resolved Hide resolved
telegram/utils/deprecate.py Outdated Show resolved Hide resolved
tests/test_animation.py Outdated Show resolved Hide resolved
tests/test_animation.py Outdated Show resolved Hide resolved
tests/test_animation.py Outdated Show resolved Hide resolved
@harshil21
Copy link
Member Author

harshil21 commented Feb 1, 2021

I would like to see another test that iterates over all modules and makes every class actually has slots.

Good idea. Although I'll have to exclude classes from filters as they are messy and we are refactoring them soon anyway.
Done.

Could go to a standalone test_slots.py. If testing for additional slots can work on class.__slots__ instead of class instances, one could also think about moving the tests for extra slots to that test file.

iirc, inspect or any other method for that matter doesn't get instance attributes without initializing the class, but I'll check that again...

  • Test that class.custom_attribute actually gives a warning

I did that for the base classes, but it's probably best I do it for all, just to be sure... (and same for the other point)
Edit: Done

@harshil21
Copy link
Member Author

harshil21 commented Feb 20, 2021

Ok so the past few commits have allowed subclassing of classes like Handler, etc without raising TGDeprecation warning for setting a new instance attribute (useful for custom {filters, handlers, persistence, Bot, Updater, Dispatcher, Handler})

The slots tests now test:

  • Extra slots
  • Missing slots (by checking the instance's __dict__)
  • Duplicate slots (if for eg. child class has same slot(s) as parent)
  • If setting a custom attribute raises a warning, and setting non custom attribute does not.

and a test_slots.py, which tests if all classes have slots, and that each have a __dict__.

Also BasePersistence uses a __dict__ internally, since we there would be a clash if we set those attributes in the slots (same name as abstract methods, which python doesn't like), I thought about assigning them as class attributes, but can't get those tests to pass.

Copy link
Member

@Bibo-Joshi Bibo-Joshi left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the updates :) Had a look and them and left some comments.

telegram/ext/filters.py Outdated Show resolved Hide resolved
telegram/ext/callbackcontext.py Show resolved Hide resolved
telegram/ext/defaults.py Show resolved Hide resolved
telegram/ext/dispatcher.py Outdated Show resolved Hide resolved
telegram/ext/dispatcher.py Outdated Show resolved Hide resolved
telegram/ext/messagequeue.py Outdated Show resolved Hide resolved
telegram/message.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
tests/test_animation.py Show resolved Hide resolved
tests/test_slots.py Outdated Show resolved Hide resolved
Copy link
Member

@Bibo-Joshi Bibo-Joshi left a comment

Choose a reason for hiding this comment

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

Hey. Thanks for the new updates. I had a look (only at the new changes for now) and left some comments. I'll have to review everything again and would also this PR to be reviewed by someone else before merging, though - tbh it became quite more involved than I had imagined 😬

telegram/bot.py Outdated Show resolved Hide resolved
telegram/error.py Outdated Show resolved Hide resolved
telegram/message.py Outdated Show resolved Hide resolved
tests/test_bot.py Show resolved Hide resolved
@harshil21
Copy link
Member Author

Hey. Thanks for the new updates. I had a look (only at the new changes for now) and left some comments. I'll have to review everything again and would also this PR to be reviewed by someone else before merging, though - tbh it became quite more involved than I had imagined 😬

I'm now gonna run all of the examples and some of my bots based on this branch to further verify that everything is working. Might also compare the memory usage for fun.

@Bibo-Joshi Bibo-Joshi added this to the v13.6 milestone Apr 30, 2021
Copy link
Member

@Poolitzer Poolitzer left a comment

Choose a reason for hiding this comment

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

Changes look good to me. They shouldn't break anything anyway, and we can add them later easily.

@Bibo-Joshi Bibo-Joshi merged commit 92ff6a8 into master May 29, 2021
@Bibo-Joshi Bibo-Joshi deleted the add-slots branch May 29, 2021 14:18
@github-actions github-actions bot locked and limited conversation to collaborators May 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add __slots__ to library objects
3 participants