Skip to content

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.

@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.

3 participants