-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
bpo-23819: asyncio: Replace AssertionError with proper excs #29894
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -706,6 +706,8 @@ def call_later(self, delay, callback, *args, context=None): | |
Any positional arguments after the callback will be passed to | ||
the callback when it is called. | ||
""" | ||
if delay is None: | ||
raise TypeError('delay must not be None') | ||
timer = self.call_at(self.time() + delay, callback, *args, | ||
context=context) | ||
if timer._source_traceback: | ||
|
@@ -717,6 +719,8 @@ def call_at(self, when, callback, *args, context=None): | |
|
||
Absolute time corresponds to the event loop's time() method. | ||
""" | ||
if when is None: | ||
raise TypeError("when cannot be None") | ||
Comment on lines
+722
to
+723
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why test only for None? What about other invalid types? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The same as above: provide descriptive error message for obvious mistakes, raise default errors for all other combinations. |
||
self._check_closed() | ||
if self._debug: | ||
self._check_thread() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Replaced asserts with exceptions in asyncio, patch by Kumar Aditya. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is redundant. A TypeError will be raised at the next line, by
self.time() + delay
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thought when I suggested the change was: better to explicitly warn about
None
argument than a little crypticunsupported 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.