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

don't call asyncio.run for async handling #94

Merged
merged 12 commits into from
Sep 6, 2024
Merged

Conversation

pmeier
Copy link
Contributor

@pmeier pmeier commented Feb 2, 2024

This Pull request fixes #93

  • I completed the issue numbers (#xx) in the sentence above. The word "fixes" should remain in front of each issue
  • My PR is tagged as draft when I'm still working on it, and I remove the draft flag when it is ready for review.
  • I added one or several changelog.md entries in the appropriate "in progress" section (not the last release one)

b - My PR fixes some issues:

(Delete this section if not relevant)

  • Each issue is well-described, well-labeled, and contains reproducible code examples.

@smarie This PR needs cleanup, but I wanted to get early feedback from you if the overall functionality is what you want.

I think it would make sense to move all the async stuff to a dedicated module or put them into utils. Any opinion here?

@smarie
Copy link
Owner

smarie commented Mar 8, 2024

Thanks @pmeier for this nice contribution.
The issue description thread looks good and the proposed solution as well.

I think that it would be a good idea, if not too difficult, to have a specific test in your test file reproducing the issue #93 explicitly, so that it can ensure non-regression in the future.

Indeed some utils_asyncio.py module could be a good idea, if enough material is to go there. But don't rush into this, you can also keep using utils.py if the amount of code for async is not too much. I let you decide what is most convenient for you.

@smarie
Copy link
Owner

smarie commented Sep 4, 2024

@pmeier do you by any chance have any progress on this ? Let me know

@pmeier
Copy link
Contributor Author

pmeier commented Sep 5, 2024

TBH, I completely forgot about this. Unfortunately, I'm swamped for the next month and will only have some dedicated time for this in October.

I think that it would be a good idea, if not too difficult, to have a specific test in your test file reproducing the issue #93 explicitly, so that it can ensure non-regression in the future.

That is already implemented.

@pmeier
Copy link
Contributor Author

pmeier commented Sep 5, 2024

@smarie I just did it now instead punting it to later. PTAL.

@pmeier pmeier marked this pull request as ready for review September 6, 2024 06:52
examples/plot_12_async.py Outdated Show resolved Hide resolved
@smarie
Copy link
Owner

smarie commented Sep 6, 2024

Amazing ! I just made a minor comment. Could you please also add a changelog entry in a new 0.10.3 section ? https://github.com/smarie/mkdocs-gallery/blob/main/docs/changelog.md

I'll release right after
Thanks !

@pmeier pmeier changed the title replace individual calls to asyncio.run with global event loop don't call asyncio.run for async handling Sep 6, 2024
@pmeier pmeier requested a review from smarie September 6, 2024 09:19
@smarie smarie merged commit de81b0c into smarie:main Sep 6, 2024
13 checks passed
@smarie
Copy link
Owner

smarie commented Sep 6, 2024

Thanks @pmeier !

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.

asyncio functionality is mostly unusable after async handling was applied
2 participants