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

Obsolete Pool API cleansed #120

Merged

Conversation

stankudrow
Copy link
Contributor

@stankudrow stankudrow commented Oct 18, 2024

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

  • only the new connection management API
  • context managers removed - there already exist one within the Pool class
  • AsyncPoolError is moved into the errors.py module - shouldn't break client code
  • all warning-causing parts removed
  • create_async_pool is renamed into the create_pool coz the project is about async anyway

The module got unburden for the next 0.2.6 release.

stankudrow pushed a commit to stankudrow/asynch that referenced this pull request Oct 18, 2024
@stankudrow stankudrow marked this pull request as draft October 18, 2024 19:26
@stankudrow stankudrow marked this pull request as ready for review October 18, 2024 19:39
@stankudrow stankudrow force-pushed the strip-obsolete-api-from-the-pool-module branch from f4ff439 to ab47f56 Compare October 29, 2024 20:40
@stankudrow
Copy link
Contributor Author

@long2ice , greetings.

The PR is rebased and ready to be merged.

@stankudrow
Copy link
Contributor Author

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

@DaniilAnichin
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

stankudrow commented Oct 30, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

@long2ice , ready to be merged.

@DaniilAnichin
Copy link
Contributor

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 long2ice merged commit 3b9062b into long2ice:dev Nov 1, 2024
1 check passed
@long2ice
Copy link
Owner

long2ice commented Nov 1, 2024

Thanks!

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.

3 participants