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

Make TelegramObject immutable #3249

Merged
merged 105 commits into from Dec 15, 2022
Merged

Make TelegramObject immutable #3249

merged 105 commits into from Dec 15, 2022

Conversation

Bibo-Joshi
Copy link
Member

@Bibo-Joshi Bibo-Joshi commented Sep 18, 2022

Currently based on #3233 so that I don't have to add the calls to super().__init__ everywhere again …

A few notes:

  • Must be adapted once TelegramObject.api_kwargs #3233 is merged to make sure that api_kwargs are immutable as well
  • In a few places (in telegram), I call the protecded _(un)freeze methods of objects to altern their values. At least in some of those cases, we could discuss if it would be desirable to instead create a copy of the object. (largue alliviated by make (Ext)Bot._insert_defaults copy instead of editing in-place #3311)
  • to_dict sill converts tuples into lists. IMO that's okay, as the dict is mutable anyway

TODO:

  • Make sure that classes that override TelegramObject.__hash__ (if that's still needed, anyway) also include self.__class_ in the hash to comply with the behavior of TelegramObject.__hash__
  • Merge after integrating API 6.3 changes
  • Remove the transition script

Checklist for PRs

  • Added .. versionadded:: version, .. versionchanged:: version or .. deprecated:: version to the docstrings for user facing changes (for methods/class descriptions, arguments and attributes)
  • Created new or adapted existing unit tests
  • Documented code changes according to the CSI standard
  • Added myself alphabetically to AUTHORS.rst (optional)
  • Added new classes & modules to the docs and all suitable __all__ s

@Bibo-Joshi Bibo-Joshi added the breaking 💥 Breaking changes label Sep 18, 2022
Copy link
Member

@harshil21 harshil21 left a comment

Choose a reason for hiding this comment

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

most of my comments are just about docs, type hinting and the None/empty tuple thing:

telegram/_bot.py Outdated Show resolved Hide resolved
docs/substitutions/global.rst Outdated Show resolved Hide resolved
telegram/_callbackquery.py Outdated Show resolved Hide resolved
telegram/_chat.py Outdated Show resolved Hide resolved
telegram/_chat.py Outdated Show resolved Hide resolved
telegram/_telegramobject.py Show resolved Hide resolved
telegram/_user.py Outdated Show resolved Hide resolved
telegram/_webhookinfo.py Outdated Show resolved Hide resolved
telegram/ext/_extbot.py Outdated Show resolved Hide resolved
telegram/ext/_extbot.py Outdated Show resolved Hide resolved
@Bibo-Joshi
Copy link
Member Author

Dedum … using tuple(stuff) if stuff else () instead of else None has effects for all classes that we pass to TG as arguments for something. I'll have to have a closer look on whether we like that or not … maybe not today though

telegram/_passport/passportfile.py Outdated Show resolved Hide resolved
telegram/_telegramobject.py Show resolved Hide resolved
telegram/_webhookinfo.py Outdated Show resolved Hide resolved
@Bibo-Joshi
Copy link
Member Author

Dedum … using tuple(stuff) if stuff else () instead of else None has effects for all classes that we pass to TG as arguments for something. I'll have to have a closer look on whether we like that or not … maybe not today though

I addressed this by making to_dict drop empty sequences.

So far, I see only one side-effect of this: if there is any class attribute where it makes a difference if we pass an empty array or nothing at all, this will be a problem. I have not found such an instance. If there is one in the future, one could override to_dict.

If there are other side-effects or concerns about this change that I did not see, please do tell. Personally, I like the consistency of having all sequence-type attributes always being tuples instead of None.

Copy link
Member

@harshil21 harshil21 left a comment

Choose a reason for hiding this comment

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

I think that approach is good. It would be weird if Telegram decides to consider () as something meaningful over nothing.

Besides the deepsource issue to be solved and deleting the temporary immutabilize.py file, this PR gets a LGTM from me!

@clot27 clot27 self-requested a review December 8, 2022 02:17
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.

LGTM

I love the freeze/unfreeze change here, good stuff

@Bibo-Joshi Bibo-Joshi merged commit b11a0c7 into master Dec 15, 2022
@Bibo-Joshi Bibo-Joshi deleted the TO-immutable branch December 15, 2022 14:00
@github-actions github-actions bot locked and limited conversation to collaborators Dec 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking 💥 Breaking changes needs wiki update 📖 PRs which warrants a wiki update after merge/release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants