python-trio / trio-asyncio

a re-implementation of the asyncio mainloop on top of Trio
Other
188 stars 37 forks source link

Fix #40 #41

Closed smurfix closed 6 years ago

smurfix commented 6 years ago

Defer creating an asyncio coroutine some of these need an actual asyncio context (=task)

njsmith commented 6 years ago

Pasting my review from chat:

hmm... I was thinking it was harder than that, because the real asyncio code actually does something different... the code inside proc.__call__ runs in the parent task's context, not necessarily the same task where __await__ gets called and iterated

but I guess you're simulating it always happening inside a task, which is probably what the aiohttp devs were expecting anyway if they didn't think of the create_task(some_function) case, so... guess that works :-)

I'd do a more detailed comment though because this totally something we're going to scratch our heads over later

ideally citing a specific example of a function that breaks without it

njsmith commented 6 years ago

Probably could use a test case too

njsmith commented 6 years ago

And a newsfragment

codecov[bot] commented 6 years ago

Codecov Report

Merging #41 into master will decrease coverage by 0.11%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #41      +/-   ##
==========================================
- Coverage   73.45%   73.33%   -0.12%     
==========================================
  Files          11       11              
  Lines        1262     1264       +2     
  Branches      165      165              
==========================================
  Hits          927      927              
- Misses        281      282       +1     
- Partials       54       55       +1
Impacted Files Coverage Δ
trio_asyncio/_version.py 100% <100%> (ø) :arrow_up:
trio_asyncio/adapter.py 76.15% <100%> (+0.37%) :arrow_up:
trio_asyncio/handles.py 81.3% <0%> (-1.87%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 2887766...1083e2c. Read the comment docs.