long2ice / asynch

An asyncio ClickHouse Python Driver with native (TCP) interface support.
https://github.com/long2ice/asynch
Apache License 2.0
185 stars 43 forks source link

Obsolete `Pool` API cleansed #120

Closed stankudrow closed 3 weeks ago

stankudrow commented 1 month ago

It is high time to remove the obsolete code from the pool.py module:

The module got unburden for the next 0.2.6 release.

stankudrow commented 3 weeks ago

@long2ice , greetings.

The PR is rebased and ready to be merged.

stankudrow commented 3 weeks ago

@DaniilAnichin , @itssimon, @DFilyushin, hello. You can have a look if you like, the API has got unburdened.

DaniilAnichin commented 3 weeks ago

The main question relevant to me, as clickhouse-sqlalchemy[asynch] user, is that versions of said library we're currently using do not have upper pin on the asynch (it is >=0.2.2 in 0.3.1, and was changed to >=0.2.2, <=0.2.4 in the 0.2.7, but 0.3.2 still has old pin), so I guess compatibility was broken in 0.2.5 already; Other than that, cleanup seems to be ok

DaniilAnichin commented 3 weeks ago

Also, there is another deprecation warning fired in the cursor tests in CI; should it be addressed, or we're testing to make sure old version still works?

stankudrow commented 3 weeks ago

Also, there is another deprecation warning fired in the cursor tests in CI; should it be addressed, or we're testing to make sure old version still works?

The one deprecation warning which leaves for the moment comes from cursors module and I didn't want to mix things into on melting PR.

stankudrow commented 3 weeks ago

The main question relevant to me, as clickhouse-sqlalchemy[asynch] user, is that versions of said library we're currently using do not have upper pin on the asynch (it is >=0.2.2 in 0.3.1, and was changed to >=0.2.2, <=0.2.4 in the 0.2.7, but 0.3.2 still has old pin), so I guess compatibility was broken in 0.2.5 already; Other than that, cleanup seems to be ok

Sorry, I am not sure I understood the idea, but I'll try to guess. In the version 0.2.5 Pool API changed and we fixed the (sort of) bug in the issue #121 . Now the connection() method works without raising AsynchPoolError exception which is like the older acquire method behaviour. But still, the API will bear these compatibility breaking changes.

Do you mean that some work should be done in the clickhouse-sqlalchemy[asynch] project? We can do it if you like, I'll see what can be done starting from this line. Perhaps we should act fast.

stankudrow commented 3 weeks ago

The main question relevant to me, as clickhouse-sqlalchemy[asynch] user, is that versions of said library we're currently using do not have upper pin on the asynch (it is >=0.2.2 in 0.3.1, and was changed to >=0.2.2, <=0.2.4 in the 0.2.7, but 0.3.2 still has old pin), so I guess compatibility was broken in 0.2.5 already; Other than that, cleanup seems to be ok

I would like to start with this issue about pyproject.toml and poetry in the clickhouse-sqlalchemy project and then fix the asynch upper version when the release v0.2.6 gets issued. Your help is appreciated.

stankudrow commented 3 weeks ago

@long2ice , ready to be merged.

DaniilAnichin commented 3 weeks ago

On the versioning and compatibility: sorry, I unfortunately tend to put stuff in such a way it's really hard to get The idea is, I don't use / install asynch directly, so I'm first & foremost interested in clickhouse-sqlalchemy working as expected; And from the looks of it, this potential 0.2.6 release would be breaking, and will require pins in my code (nothing critical, but still) So yes, I think it would be great to ensure proper upper pins on the clickhouse-sqlalchemy side; I will check your draft PR there about pyproject and poetry in the nearest future

long2ice commented 3 weeks ago

Thanks!