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

Add asyncio events module #815

Merged
merged 6 commits into from Apr 15, 2021
Merged

Add asyncio events module #815

merged 6 commits into from Apr 15, 2021

Conversation

bdraco
Copy link
Contributor

@bdraco bdraco commented Apr 6, 2021

This is the module being used in Home Assistant and pysonos (https://github.com/amelchio/pysonos/blob/master/pysonos/events_asyncio.py) with the extras stripped out modified to work on python 3.5+.

@bdraco bdraco force-pushed the events_asyncio branch 11 times, most recently from 8a96ad4 to 3893a8d Compare April 6, 2021 05:49
@bdraco
Copy link
Contributor Author

bdraco commented Apr 6, 2021

There is a case where I/O can happen in the event loop discovered in home-assistant/core#48732 though which affects twisted as well.

I'll push another change to fix that but twisted likely needs a similar fix to run it in twisted.internet.threads.deferToThread

@bdraco bdraco marked this pull request as ready for review April 7, 2021 03:29
soco/events_asyncio.py Outdated Show resolved Hide resolved
soco/events_asyncio.py Outdated Show resolved Hide resolved
@pwt pwt self-assigned this Apr 8, 2021
@pwt
Copy link
Contributor

pwt commented Apr 9, 2021

Hi @bdraco:

Thanks for this great contribution. I'm optimistically targeting its inclusion in the upcoming v0.22 release (17th April).

I'm going to continue to review and make comments. Please excuse me making comments in multiple passes ... I only have a superficial knowledge of asyncio.

Edit: Also, I realise that some of the review comments could equally apply to some of the existing code; I just want to make new code as clean as possible.

@pwt pwt added this to the 0.22.0 milestone Apr 9, 2021
soco/events_asyncio.py Outdated Show resolved Hide resolved
soco/events_asyncio.py Outdated Show resolved Hide resolved
soco/events_asyncio.py Outdated Show resolved Hide resolved
soco/events_asyncio.py Outdated Show resolved Hide resolved
soco/events_asyncio.py Outdated Show resolved Hide resolved
@pwt
Copy link
Contributor

pwt commented Apr 9, 2021

I've pushed the changes I proposed in my review to your branch (a0303a6). Feel free to review and revert, of course.

@pwt pwt added the Enhancement label Apr 9, 2021
@bdraco
Copy link
Contributor Author

bdraco commented Apr 9, 2021

I've pushed the changes I proposed in my review to your branch (a0303a6). Feel free to review and revert, of course.

Nice! LGTM 👍

@pwt pwt requested a review from KennethNielsen April 10, 2021 10:45
@pwt
Copy link
Contributor

pwt commented Apr 10, 2021

I think this is ready to merge, but I wouldn't mind another set of eyes on the changes to events_base.py in particular. (@KennethNielsen if you have time perhaps you could take a quick look. No problem if not.)

In the absence of further comments, I plan to merge this in the next few days, for inclusion in v0.22.

@pwt pwt removed the request for review from KennethNielsen April 15, 2021 08:02
@pwt pwt merged commit aa6765b into SoCo:master Apr 15, 2021
@bdraco
Copy link
Contributor Author

bdraco commented Apr 15, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants