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 AnyIO and type-hints; other modernization. #22

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JaagupAverin
Copy link

@JaagupAverin JaagupAverin commented May 27, 2024

Hey! Thanks for this project, I've found good use for it in a library I'm working on.

However my project has the following requirements:

  • AnyIO as async framework
  • Typed
  • Support for PEP621 pyproject.toml spec

Since it ended up being quite a large rewrite (compared to the size of this library), I would understand not wanting to deal with this (breaking) PR, in which case I can create an alternative package instead. Otherwise, feedback and changes are welcome.

Full list of changes:

  • Rewrite FairAsyncRLock to use AnyIO so it can be used with asyncio, trio, etc.
  • Rewrite tests using AnyIO API
    • Some tests that were found redundant, or not applicable to AnyIO were not rewritten;
  • Add [project] to pyproject.toml, as expected by some modern packaging tools (e.g Rye)
  • Update requirements.txt as needed by AnyIO
  • Add project classifiers
  • Bump Python requirement to >=3.8
  • Bump project version to 1.1.0

@Joshuaalbert Joshuaalbert self-requested a review May 27, 2024 15:08
@Joshuaalbert
Copy link
Owner

@JaagupAverin thanks for the initiative. I'm open to including more support for this, however would like to be non-disruptive to current users. Some issues with the current PR would be that it has >= 3.11, which would be potentially disruptive. Having AnyIO and trio as dependencies would be disruptive to a lesser extend and potentially liveable with. While I do encourage all async codebase to upgrade to 3.12 (for performance boosts) I can't rule out some stragglers yet to upgrade. Is there a strict requirement of 3.11 for AnyIO?

Also, can you say a little more about the reasoning, and potential benefits of using AnyIO?

@JaagupAverin
Copy link
Author

JaagupAverin commented May 27, 2024

As for Python 3.11 - it is not a strict requirement. Reason for it being that the latest version of AnyIO requires Python >=3.8, and when I began to adjust this lib for my project I hadn't considered creating a PR yet so I liberally used my favourite syntax from Python 3.10 and Python 3.11. The syntax can be rewritten and I'm sure there's a compromise version of AnyIO for Python 3.7.
edit: Personally, I would prefer to increment the Python version often with minor releases. Python 3.7 will be quite ancient in a few years and mixing old Python requirements with newer libraries is a hassle and a half. Older users can always pin their version, while new users can use the latest release. For larger frameworks, bumping the version is hard/impossible outside of major releases, but for a small library such as this I would see it as an easy transition.

My support for AnyIO is driven by my support for trio over asyncio. This debate is purely subjective, but libraries have the option to appease both clubs by using AnyIO, which natively supports both.

@Joshuaalbert
Copy link
Owner

@JaagupAverin Could be nice to address this again. anyio dependency is fine. Do you have bandwidth to rebase on main latest, and get unittests passing?

* Rewrite FairAsyncRLock to use AnyIO so it can be used with asyncio, trio, etc.
* Rewrite tests using AnyIO API
* Add [project] to pyproject.toml, as expected by some modern packaging tools (e.g Rye)
* Update requirements.txt as needed by AnyIO
* Add project classifiers
* Bump Python requirement to >=3.11
* Bump project version to 1.1.0
@JaagupAverin
Copy link
Author

JaagupAverin commented Oct 23, 2024

Hey. I did rebase onto main and lowered Python requirement to 3.8, but 3.7 is hard to reach as AnyIO dropped 3.7 support at its 4.0.0 version, and has had a lot of fixes and improvements since, so I would not compromise on this further.

I've currently ended up integrating my fork of this project directly to my project, where I've rewritten the tests almost entirely. While the new tests have 100% code coverage and cover all situations I can think of, they weren't rewritten 1:1 as the tests you had, as some of them weren't relevant for AnyIO (different API) or seemed to be covered by other cases already. I can look into re-implementing some of the tests that were removed over the upcoming weeks if I have time and you think they're still necessary.

@Joshuaalbert
Copy link
Owner

I think we can bump this project to 3.10. Most async project are probably updating fairly regularly to take advantage of 3.12 improvements. I wouldn't have a problem bumping to 3.10 so that nicer annotation syntax is available. If anyone is using an older version then can pin to their respective version. With that if you want to start using 3.10+ syntax that's fine with me.

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