bpo-23819: asyncio: Replace AssertionError with proper excs#29894
bpo-23819: asyncio: Replace AssertionError with proper excs#29894asvetlov merged 5 commits intopython:mainfrom
Conversation
|
cc @gvanrossum This only changes AssertionError with exceptions, this does not removes all asserts as you suggested on bpo |
0f78468 to
52c602d
Compare
|
Apologies, I should have been clearer in my review. My point wasn't that you should use double-quotes instead of single-quotes — my point was that Python's
Rather than
|
52c602d to
3d95ee3
Compare
Done |
Thanks! |
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @asvetlov: please review the changes made to this pull request. |
|
Thanks! |
|
Congrats Kumar, and thanks Andrew for guiding Kumar towards success here!
|
| if delay is None: | ||
| raise TypeError('delay must not be None') |
There was a problem hiding this comment.
It is redundant. A TypeError will be raised at the next line, by self.time() + delay.
There was a problem hiding this comment.
My thought when I suggested the change was: better to explicitly warn about None argument than a little cryptic unsupported operand type(s) for +: 'NoneType' and 'int'.
I believe not only exception type but also a proper message can help users to avoid silly mistakes.
There was a problem hiding this comment.
Why is None special for this argument? I do not know any precedents of checkinjg only for None if other types can be invalid too. It looks like an antipattern to me. We either check for allowed types, or do not check either and let an error be raised later when perform an unsupported operation.
| if when is None: | ||
| raise TypeError("when cannot be None") |
There was a problem hiding this comment.
Why test only for None? What about other invalid types?
There was a problem hiding this comment.
The same as above: provide descriptive error message for obvious mistakes, raise default errors for all other combinations.
https://bugs.python.org/issue23819