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

AsyncIOScheduler: add a method to retrieve event loop #458

Closed
wants to merge 1 commit into from

Conversation

MainRo
Copy link
Collaborator

@MainRo MainRo commented Sep 6, 2019

This commit adds a getter to retrieve the event loop from a, asyncio scheduler. This is typically useful when using some asyncio code/library in a subscription function.
We could also add such getters in the other eventloop schedulers.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 92.904% when pulling e98c698 on MainRo:feature/aio_get_event_loop into 19b1908 on ReactiveX:master.

@MainRo
Copy link
Collaborator Author

MainRo commented Oct 1, 2019

@erikkemperman @jcafhe any opinion of this PR (and some of the others that are open) ?
@dbrattli is mistakenly added Codacy in the checks (btw I still do not understand how github allows this from third party apps, without explicit consent). I now removed it, can you force a re-run of the checks or do we have to update the PR with a new commit ?

Copy link
Collaborator

@jcafhe jcafhe left a comment

Choose a reason for hiding this comment

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

@MainRo I've have not a clear opinion about this since I do not use asyncio, but if it's helpfull for you & others, that's ok for me 👍

@dbrattli
Copy link
Collaborator

I feel unsure about this. Both because it makes the API surface uneven for schedulers (not all schedulers have it) and you are never sure what scheduler you will get. In subscribe you get a Scheduler not an AsyncIOScheduler, so you would need to test it runtime. I think it might be better to use asyncio.get_running_loop().

using the get_running_loop() function is preferred to get_event_loop() in coroutines and callbacks.

@MainRo
Copy link
Collaborator Author

MainRo commented Oct 19, 2019

I feel unsure about this. Both because it makes the API surface uneven for schedulers (not all schedulers have it) and you are never sure what scheduler you will get. In subscribe you get a Scheduler not an AsyncIOScheduler, so you would need to test it runtime. I think it might be better to use asyncio.get_running_loop().

I understand your concerns. I agree that this API would only be meaningful for event loop schedulers, and the type of the result would be different for each scheduler.
On the other way, I prefer to provide the event loop explicitly whenever possible rather than retrieving the current one.
So I am not sure if it is better to keep the API as is, and let developers deal themselves with the event loop or add this new method that would not be consistent with all schedulers.

@MainRo
Copy link
Collaborator Author

MainRo commented Dec 27, 2019

closing this PR for now since I do not have a better proposal for now.

@MainRo MainRo closed this Dec 27, 2019
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.

None yet

4 participants