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

[WIP] Transition to AsyncIO #244

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

nmanumr
Copy link

@nmanumr nmanumr commented Aug 25, 2024

As discussed in #243, This PR Ports the whole codebase to asyncio.

Todos:

  • Migrate tests to asyncio.

Transitioned all major functions to asynchronous counterparts with `async` and `await` keywords. Replaced `put` with `put_nowait` for non-blocking event handling in input queues, and replaced `time.wait` with `asyncio.sleep` for better performance. Added import for `asyncio` where necessary, and removed unused imports to clean up the code.
@nmanumr nmanumr marked this pull request as draft August 25, 2024 11:46
Introduced new async compatibility functions in pcbasic/compat/asyncio.py. Refactored multiple methods across different modules to include `async` and `await` keywords. This enhances the program's handling of asynchronous operations and improves overall efficiency.
@robhagemans
Copy link
Owner

Hi @nmanumr, apologies I haven't had time to look into the PR in detail and indeed to try the code. Part of the problem is that to be honest I'm unfamiliar with the async style of concurrency in Python, and this is obviously a large PR that pervades the whole code base, so would need to brush up, and there's a lot to test manually (e.g. ON KEY and other event driven structures that are difficult to cover in unit tests). Currently I'm mentally more in maintenance mode with this code with even there a backlog building up (the MOD fix definitely needs to go through e.g.). I guess I was hoping support for the web canvas could be built in a more limited way that didn't take the whole code asynchronous...

I do still intend to try this and play with it, just wanted to be open about my limited time/focus on this project and not create unrealistic expectations.

@nmanumr
Copy link
Author

nmanumr commented Oct 15, 2024

Hi, @robhagemans, I completely forgot that I had opened the PR. I appreciate your openness, and I’ve been thinking about the number of changes as well. Initially, I didn’t expect it to turn into such a large change, so I think it would be worthwhile to look for a simpler solution. In the meantime, let’s close the PR, as this may not be the right approach. I’ll open a new PR once I come up with a simpler solution.

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