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

Fix TypeError while unpickling TelegramError (and children) #2106

Merged
merged 5 commits into from
Oct 4, 2020
Merged

Fix TypeError while unpickling TelegramError (and children) #2106

merged 5 commits into from
Oct 4, 2020

Conversation

Delgan
Copy link
Contributor

@Delgan Delgan commented Oct 2, 2020

Hi.

This is a possible fix for #2105.

Let me know if you think this can be improved in any way.

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.

Hi. Thanks for the quick PR! I left two comments for you.

telegram/error.py Outdated Show resolved Hide resolved
tests/test_error.py Outdated Show resolved Hide resolved
telegram/error.py Outdated Show resolved Hide resolved
tests/test_error_pickling.py Outdated Show resolved Hide resolved
@tsnoam
Copy link
Member

tsnoam commented Oct 3, 2020

@Bibo-Joshi If we ever introduce a new exception which inherits from TelegramError, would it be covered by the unitests here?

Maybe we also need a unitests which looks at telegram.error.TelegramError.__subclasses__() and make sure that all subclasses are known and properly covered?

@Delgan Delgan requested a review from Bibo-Joshi October 3, 2020 07:55
@Bibo-Joshi
Copy link
Member

@Bibo-Joshi If we ever introduce a new exception which inherits from TelegramError, would it be covered by the unitests here?

Maybe we also need a unitests which looks at telegram.error.TelegramError.__subclasses__() and make sure that all subclasses are known and properly covered?

It wouldn't. But we might not be directly inheriting from TGError, so we would need a recursive loop. Should be doable, just not with parametrize. I can have a look, when I'm back at a keyboard and had some sleep :D

Copy link
Member

tsnoam commented Oct 3, 2020

@Bibo-Joshi A basic and very naive unitest, yet serving the purpose would use the subclasses method in order to retrieve a list of methods and compare it to a pre-known const.
If the lists are not equal it means that we've added a new Exception and thus we need to make sure it has a unitest of its own (of course a comment all these would require a nice comment so people would know why we do this "stupid" things in the test)

Basically the idea is to make us remember that we need to implement un/pickling...

@Bibo-Joshi
Copy link
Member

Added the "meta-test" and in the process found that there is a TelegramDecryptionError in passport/credentials.py :D That one in fact is a bit tricky as it passes TelegramDecryptionError: {message} to super().__init__. But I didn't want to fiddle with that behaviour, so I just added a self._msg that saves the actual message on init and is returned on __reduce__.

@Bibo-Joshi Bibo-Joshi merged commit e67b995 into python-telegram-bot:master Oct 4, 2020
@Bibo-Joshi
Copy link
Member

Thank your for your contribution @Delgan :)

@github-actions github-actions bot locked and limited conversation to collaborators Oct 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants