Skip to content
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

Replace yield from with await... #283

Merged
merged 9 commits into from
May 1, 2018
Merged

Replace yield from with await... #283

merged 9 commits into from
May 1, 2018

Conversation

terricain
Copy link
Collaborator

#281 Point 2 Replace yield from with await.

Well this resulted in a nastier diff than I expected :D.

Have replaced the majority of the yield from with await and @asyncio.coroutine with async

This is more a placeholder for when you want to purge the yield from. Its not 100% finished but if you fancy reading through it, by all means :)

# allow consistent errors
self._cursor = None
self._weak = None

def __iter__(self):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah this stopped working... not too sure why

@terricain terricain mentioned this pull request Apr 22, 2018
@codecov
Copy link

codecov bot commented Apr 22, 2018

Codecov Report

Merging #283 into master will decrease coverage by 0.8%.
The diff coverage is 95.43%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #283      +/-   ##
==========================================
- Coverage   92.93%   92.12%   -0.81%     
==========================================
  Files           9        9              
  Lines        1161     1118      -43     
  Branches      173      158      -15     
==========================================
- Hits         1079     1030      -49     
- Misses         53       61       +8     
+ Partials       29       27       -2
Impacted Files Coverage Δ
aiomysql/sa/result.py 90.86% <100%> (-0.4%) ⬇️
aiomysql/sa/engine.py 88.5% <100%> (-11.5%) ⬇️
aiomysql/pool.py 94.93% <89.65%> (-1.32%) ⬇️
aiomysql/sa/connection.py 92.07% <92.72%> (-0.24%) ⬇️
aiomysql/utils.py 84.76% <96%> (+2.94%) ⬆️
aiomysql/cursors.py 93.9% <96.49%> (-0.11%) ⬇️
aiomysql/sa/transaction.py 95.83% <96.66%> (-0.17%) ⬇️

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 e95df74...72de31b. Read the comment docs.

@jettify
Copy link
Member

jettify commented Apr 23, 2018

Can you please split this PR in 2 parts: just port tests, without library change and subsequent PR with library change? Should be easy to do, since you have one commit per file. I can help you with splitting.

Reason for 2 PR, I just want to be extra caution that move to async def does not introduce new issue.

@jettify
Copy link
Member

jettify commented Apr 23, 2018

Other question, is what minimum version of python3.5 we should support?

@terricain
Copy link
Collaborator Author

Sure makes much more sense. Any recommendations for splitting the PR, as normally I resort to mashing git commands until it looks ok 😄.

As for Python3.5 versions, I don't know enough about the differences between patch versions, do you know of anything that we should be cautious of? Otherwise, 3.5.2 is on Ubuntu, so assuming that has a fair market share, that's not a bad minimum.

@jettify
Copy link
Member

jettify commented Apr 24, 2018

I was thinking about 3.5.3, since older versions has annoying bug with with asyncio.get_event_loop(), it may return other loop not currently running in some strange cases.

@terricain
Copy link
Collaborator Author

LOL yeah that is an annoying bug. Sure 3.5.3 is fine by me, im on 3.6 :D

@jettify
Copy link
Member

jettify commented May 1, 2018

cherry picked tests locally to make sure all set

@jettify jettify merged commit 83252b3 into aio-libs:master May 1, 2018
@jettify
Copy link
Member

jettify commented May 1, 2018

thanks!

@jettify jettify mentioned this pull request May 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants