Skip to content

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

Merged
merged 5 commits into from
Dec 6, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Lib/asyncio/base_events.py
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Comment on lines +709 to +710
Copy link
Member

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.

Copy link
Contributor

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

Copy link
Member

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.

timer = self.call_at(self.time() + delay, callback, *args,
context=context)
if timer._source_traceback:
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why test only for None? What about other invalid types?

Copy link
Contributor

Choose a reason for hiding this comment

The 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()
Expand Down
7 changes: 4 additions & 3 deletions Lib/asyncio/events.py
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,6 @@ class TimerHandle(Handle):
__slots__ = ['_scheduled', '_when']

def __init__(self, when, callback, args, loop, context=None):
assert when is not None
super().__init__(callback, args, loop, context)
if self._source_traceback:
del self._source_traceback[-1]
Expand Down Expand Up @@ -661,7 +660,8 @@ def get_event_loop(self):
def set_event_loop(self, loop):
"""Set the event loop."""
self._local._set_called = True
assert loop is None or isinstance(loop, AbstractEventLoop)
if loop is not None and not isinstance(loop, AbstractEventLoop):
raise TypeError(f"loop must be an instance of AbstractEventLoop or None, not '{type(loop).__name__}'")
self._local._loop = loop

def new_event_loop(self):
Expand Down Expand Up @@ -745,7 +745,8 @@ def set_event_loop_policy(policy):

If policy is None, the default policy is restored."""
global _event_loop_policy
assert policy is None or isinstance(policy, AbstractEventLoopPolicy)
if policy is not None and not isinstance(policy, AbstractEventLoopPolicy):
raise TypeError(f"policy must be an instance of AbstractEventLoopPolicy or None, not '{type(policy).__name__}'")
_event_loop_policy = policy


Expand Down
4 changes: 4 additions & 0 deletions Lib/test/test_asyncio/test_base_events.py
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,8 @@ def cb():
self.assertIsInstance(h, asyncio.TimerHandle)
self.assertIn(h, self.loop._scheduled)
self.assertNotIn(h, self.loop._ready)
with self.assertRaises(TypeError, msg="delay must not be None"):
self.loop.call_later(None, cb)

def test_call_later_negative_delays(self):
calls = []
Expand Down Expand Up @@ -286,6 +288,8 @@ def cb():
# tolerate a difference of +800 ms because some Python buildbots
# are really slow
self.assertLessEqual(dt, 0.9, dt)
with self.assertRaises(TypeError, msg="when cannot be None"):
self.loop.call_at(None, cb)

def check_thread(self, loop, debug):
def cb():
Expand Down
8 changes: 2 additions & 6 deletions Lib/test/test_asyncio/test_events.py
Original file line number Diff line number Diff line change
Expand Up @@ -2322,10 +2322,6 @@ def callback(*args):
self.assertIsNone(h._callback)
self.assertIsNone(h._args)

# when cannot be None
self.assertRaises(AssertionError,
asyncio.TimerHandle, None, callback, args,
self.loop)

def test_timer_repr(self):
self.loop.get_debug.return_value = False
Expand Down Expand Up @@ -2592,7 +2588,7 @@ def test_set_event_loop(self):
policy = asyncio.DefaultEventLoopPolicy()
old_loop = policy.get_event_loop()

self.assertRaises(AssertionError, policy.set_event_loop, object())
self.assertRaises(TypeError, policy.set_event_loop, object())

loop = policy.new_event_loop()
policy.set_event_loop(loop)
Expand All @@ -2608,7 +2604,7 @@ def test_get_event_loop_policy(self):

def test_set_event_loop_policy(self):
self.assertRaises(
AssertionError, asyncio.set_event_loop_policy, object())
TypeError, asyncio.set_event_loop_policy, object())

old_policy = asyncio.get_event_loop_policy()

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Replaced asserts with exceptions in asyncio, patch by Kumar Aditya.