[bug fix] add do_async to cover inlining multiple async calls in a do… #152
Conversation
JLessinger
force-pushed
the
do-async-fix
branch
from
d590c5c to
899e8b1
Compare
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #152 +/- ##
==========================================
+ Coverage 97.41% 97.48% +0.07%
==========================================
Files 4 4
Lines 464 556 +92
==========================================
+ Hits 452 542 +90
- Misses 12 14 +2
|
|
I forgot you opened this before I released 0.15.0. Anyway, I'll release it in 0.15.1. |
|
There's also this syntax, which I wasn't familar with since I haven't worked with async-python at all really. async for x in foo()Could/should we try to support this? |
As far as I understand, I think the cause of this bug is that Python seems to turn our synchronous iterators into async iterators if multiple |
JLessinger
force-pushed
the
do-async-fix
branch
2 times, most recently
from
265f996 to
4400902
Compare
| Ok(1) | ||
| for x in await foo() | ||
| for y in await bar() | ||
| ) |
There was a problem hiding this comment.
I'm not sure about code like this, I don't think it renders properly.
It's fine for now
There's an issue open to setup docs and we can look into it when that's being done. Perhaps we'd need to follow Sphinx's formatting rules for docstrings to render properly.
| return next(gen) | ||
| except DoException as e: | ||
| out: Err[E] = e.err # type: ignore | ||
| return out |
There was a problem hiding this comment.
Why don't we just replace do with this implementation? This seems to cover both cases, I don't see an obvious need to have both. Just having the one that works in all cases seems like the better choice.
Aside from this extra isinstance check at runtime,
There was a problem hiding this comment.
Yeah interesting idea. You really need the two "front-ends" because one is async and has to be awaited. It would be weird to have to await every do call even if you're not using any actual asyncio.
Question is how much implementation they can share, and I think the answer is not much. If you look at them side by side, they're pretty different. I could abstract out the try... except DoException but I think that's really overdoing abstraction. The duplicated code amounts to maybe 4 lines.
There was a problem hiding this comment.
You're right, I didn't consider that Python necessitates awaits for async stuff and it would look weird for non-async.
JLessinger
force-pushed
the
do-async-fix
branch
3 times, most recently
from
3883fd4 to
9a19c5e
Compare
|
@francium , updated, PTAL |
There was a problem hiding this comment.
Looks good to me. Could you have a look at my two comments prior to merging please.
I don't time currently to sit down and play with this more, I'd like to do that prior to releasing just to familiarize myself with some of Python's async stuff more. But we don't need to hold up this PR, I'll do some testing after it's merged.
| Ok(len(x) + int(y) + z) | ||
| for x in await aget_resx(True) | ||
| for y in await aget_resy(True) | ||
| for z in get_resz(True) |
There was a problem hiding this comment.
Random question, does it at all depend on the order in which we have async and non-async lines? For example,
for x in foo(...)
for y in await bar(...)
vs
for y in await bar(...)
for x in foo(...)
I don't think it would, but perhaps good to verify in case it's some edge case there as well.
| for x in await aget_resx(is_suc1) | ||
| for y in await aget_resy(is_suc2) | ||
| for z in get_resz(is_suc3) | ||
| for w in await process_xyz(x, y, z) |
There was a problem hiding this comment.
Could you, either in this PR, or in a separate PR, document this behavior as well, the fact you can use previous values in subsequent lines,
for a in A(...)
for b in B(a)
for c in C(b)
If someone has used do-notation in another language they may be familiar, but it may be unintuitive for new comers.
JLessinger
force-pushed
the
do-async-fix
branch
2 times, most recently
from
f59a1a1 to
5c76e89
Compare
JLessinger
force-pushed
the
do-async-fix
branch
from
5c76e89 to
ebae79b
Compare
|
@francium done, but I don't have write access. |
|
Sorry, I thought PR creators would be allowed to hit merge after PR was approved. Merging. Thank for the changes. Also, for future, best to avoid force pushes after PR has started getting reviews, otherwise you can't as easily view changes since last review :) (technically you can but, just easier overall if we avoid force pushes once reviews start) |
Due to subtle behavior in the way Python creates generators,
do()breaks if you inline multipleawaitcalls. If you either await beforedo()or only inline oneawait,do()works as expected. This PR makes two changes:do_async()which can handle the broken case (multiple inlined awaits). Also can handle any synchronous generator including one inlined await.do()case pointing todo_async().Docs are updated.