rethinkdb / rethinkdb-python

Python driver for RethinkDB
https://rethinkdb.com/api/python/
Apache License 2.0
64 stars 35 forks source link

Remove loop params that break in Python 3.10 #296

Closed srh closed 1 year ago

srh commented 1 year ago

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

codeclimate[bot] commented 1 year ago

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

View more on Code Climate.

lsabi commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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

lsabi commented 1 year ago

@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 commented 1 year ago

@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: https://github.com/rethinkdb/rethinkdb-python/pull/303/commits/ef08ba2a89108fde70227012b253e225e7933cf7 will hit the reusable_waiter wait_for and the open_connection cases.

lsabi commented 1 year ago

@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 commented 1 year ago

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.