#179: Added general telegram exceptions handling. #180
Conversation
| def exception_handler(self, loop, context): | ||
| click.echo(context.get('exception')) | ||
| loop.default_exception_handler(context) | ||
| loop.stop() |
There was a problem hiding this comment.
Should we call self.stop() here?
There was a problem hiding this comment.
Unfortunately, calling self.stop() here will cause an exception "cannot close running event loop". Also, these two methods are semantically different: self.loop.close() (inside self.stop()) tries to close event loop immediately (which is not possible in exception handler), but loop.stop() marks loop to be closed ASAP. I was thinking about replacing self.loop.close() by self.loop.stop(), but I'm not sure if this issue a good place for such a proposal 😄
There was a problem hiding this comment.
I think self.stop wasn't done the right way. There's another issue related to it (#175). So yep, this PR isn't the best place to do discuss it indeed, but I think we should have at least a commend here saying that the stop/close should be done by another self.stop().
There was a problem hiding this comment.
Ok, so I'll add a comment in this PR, and also will try to fill a PR for the #175 to stop the event loop gracefully.
bottery/telegram/engine.py
Outdated
| self.platform, | ||
| updates | ||
| ) | ||
| raise Exception(msg) |
There was a problem hiding this comment.
Should we create a specific exception? Something like PlatformError? What do you think?
There was a problem hiding this comment.
Sure, I'll change an Exception to something more meaningful 😄
|
Hi @kaduev13, thanks for the PR. I made some comments, could you take a look at them? Also, what do you think about adding some tests? |
|
Hi @rougeth! Thank you for the review, I'll make the fixes tomorrow. About the tests – yeah, I'll add them, sorry for not doing it in the initial PR – I tried to find |
| self.message = message | ||
|
|
||
| def __str__(self): | ||
| return '[{}] {}'.format(self.platform, self.message) |
There was a problem hiding this comment.
I started questioning myself if the [{}] {} is a pattern that we should keep following. In this case, I think we don't need the self.platform, the exception itself should be able to give all the info we need. Maybe class TelegramException(PlatformException) is a good idea. What do you think?
I added asyncio loop exception handler and Telegram exception handling on top of it.
Closes: #179