-
-
Notifications
You must be signed in to change notification settings - Fork 338
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
Clearer error message if async context manager used synchronously #212
Conversation
f4878c3
to
047c305
Compare
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.
two nitpicks
trio/_util.py
Outdated
@@ -135,6 +136,13 @@ def __init__(self, func, args, kwds): | |||
if sys.exc_info()[1] is not value: | |||
raise | |||
|
|||
def __enter__(self): | |||
raise RuntimeError("The function `{}` create an async context manager, use with `async with` and not `with`.".format(self._func_name)) |
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.
"use 'async with {func_name}(...)', not 'with {func_name}(...)'".format(name=self._func_name)"
(and 80 column limit!)
trio/_util.py
Outdated
def __enter__(self): | ||
raise RuntimeError("The function `{}` create an async context manager, use with `async with` and not `with`.".format(self._func_name)) | ||
|
||
def __exit__(self): |
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.
# pragma: no-cover
Hum, also unawaited coroutines. I need to fix the test. |
047c305
to
386b4e5
Compare
Codecov Report
@@ Coverage Diff @@
## master #212 +/- ##
==========================================
+ Coverage 99.01% 99.06% +0.04%
==========================================
Files 59 63 +4
Lines 8012 8407 +395
Branches 569 606 +37
==========================================
+ Hits 7933 8328 +395
Misses 62 62
Partials 17 17
Continue to review full report at Codecov.
|
Otherwise it says it does not have `__enter__` which is obvious for advanced pythonista. Still a tiny bit clearer is likely better. I guess this _might_ fool static type checkers, but I'm unaware of any that would flag that.
386b4e5
to
ab22313
Compare
If it does create a problem for static type checkers then it should be fixable by adding an explicit annotation to Something else we might want to think about is coming up with a more standard/comprehensive way of doing this kind of checking, like a decorator we can put on all our context managers and iterators. But that doesn't need to block this; this case is a bit special because we can give a nice error message. |
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.
Sorry, missed that this was ready for review.
Two tiny nitpicks but otherwise looks good.
trio/_util.py
Outdated
raise RuntimeError("use 'async with {func_name}(...)', not 'with {func_name}(...)'".format(func_name=self._func_name)) | ||
|
||
def __exit__(self): | ||
assert False, """Never called, but should be defined""" # pragma: no-cover |
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.
Should be no cover
, not no-cover
; it's not actually being recognized right now :-). Also better to put two spaces before the #
, and to move it up one line, so:
def __exit__(self): # pragma: no cover
trio/_core/tests/test_run.py
Outdated
with pytest.raises(RuntimeError) as excinfo: | ||
on = _core.open_nursery() | ||
with on as nursery: | ||
pass # pragma: no-cover |
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.
Also needs an extra space before #
and removal of the -
Oo, I figured out how to fix the nitpicks through github. Let's see what the CI thinks... |
Thanks! |
Thanks ! |
Otherwise it says it does not have
__enter__
which is obvious foradvanced pythonista. Still a tiny bit clearer is likely better.
I guess this might fool static type checkers, but I'm unaware of any
that would flag that.