Skip to content

Remove loop params that break in Python 3.10 #296

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

Merged
merged 2 commits into from
Oct 9, 2023
Merged

Remove loop params that break in Python 3.10 #296

merged 2 commits into from
Oct 9, 2023

Conversation

srh
Copy link
Contributor

@srh srh commented Oct 4, 2023

Addresses #264. This problem could be reproduced with the README.

The loop parameter as documented in 3.9 is optional. In 3.10 it is removed.

https://docs.python.org/3.9/library/asyncio-stream.html#asyncio.open_connection

Checklist

srh added 2 commits October 4, 2023 14:06
Silences warning (or perhaps legit error, now)
In Python 3.9, they are optional, and we default to the active event loop.
@codeclimate
Copy link

codeclimate bot commented Oct 4, 2023

Code Climate has analyzed commit 10e9545 and detected 0 issues on this pull request.

View more on Code Climate.

@lsabi
Copy link
Contributor

lsabi commented Oct 5, 2023

@srh Thanks for the changes. I have the question below

https://docs.python.org/3/library/asyncio-task.html#asyncio.wait_for does remove the loop parameter, while it's present in the 3.9 version. Wouldn't it be better to remove it, providing compatibility with both 3.9 and 3.12 ?

@srh
Copy link
Contributor Author

srh commented Oct 5, 2023

@lsabi It is an optional parameter, and it's only used for interacting with multiple event loops. Basically, the only way it would happen to change things is if somebody tried to use the coroutine across multiple event loops.

However, let me double check nothing breaks on Python 3.5. I don't think it's a required parameter, but I still need to get Python 3.5 working. (I tried building from source yesterday, and pip got segfaults, which blocked me.)

@srh
Copy link
Contributor Author

srh commented Oct 5, 2023

I can confirm (through testing) that the loop parameters are optional in Python 3.5.

@srh srh mentioned this pull request Oct 6, 2023
@lsabi
Copy link
Contributor

lsabi commented Oct 8, 2023

@srh Starting from python 3.10 and upwards, there are no optional parameters, just awaitable and timeout plus no **kwargs nor *args. My concern is related to this function that would break on 3.10+ systems. How did you test it? Some parts are still not working with python 3.XX so I'm a little bit in difficoulty testing it on my system with python 3.11.4.

Thanks

@srh
Copy link
Contributor Author

srh commented Oct 9, 2023

@lsabi I tested the code (for 3.5) by running it with Python 3.5. This confirmed that it uses the currently running event loop when the loop parameter is None. (I didn't exercise the timeout error branch but it does exercise the other use of wait_for in reusable_waiter with a cursor.) I've also run it on 3.8, 3.10, 3.11, and 3.12.

The asyncio example in the readme, or the updated version in the other PR: ef08ba2 will hit the reusable_waiter wait_for and the open_connection cases.

@lsabi lsabi merged commit b8441ed into master Oct 9, 2023
@lsabi
Copy link
Contributor

lsabi commented Oct 9, 2023

@srh I didn't manage to make it work locally, but it's been a while since I last run a python program on my personal machine, so I may need some updates and setup.

Since you tested with 3.10+ versions I'm going to accept it and merge it

@srh
Copy link
Contributor Author

srh commented Oct 9, 2023

To be clear it was tested with the changes in #303.

Used a source distribution of Python 3.5, and on Ubuntu Linux, I configured and built the code with a custom --prefix=/home/srh/prefix35 on the configure line, and then added /home/srh/prefix35/bin to the path. Used pip3.5 install -r requirements.txt and make prepare as the README suggests, than ran the test program I made with python3.5 test.py.

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