Skip to content

redis.connection.ConnectionPool API change: get_connection does not accept arguments#2294

Merged
auvipy merged 3 commits intocelery:mainfrom
woutdenolf:fix_2293
May 7, 2025
Merged

redis.connection.ConnectionPool API change: get_connection does not accept arguments#2294
auvipy merged 3 commits intocelery:mainfrom
woutdenolf:fix_2293

Conversation

@woutdenolf
Copy link
Copy Markdown
Contributor

Closes #2293

@Nusnus Nusnus self-requested a review May 3, 2025 10:56
@auvipy auvipy merged commit f78c440 into celery:main May 7, 2025
42 checks passed
@auvipy auvipy added this to the 5.5.0 milestone May 12, 2025
@sevdog
Copy link
Copy Markdown

sevdog commented Jun 3, 2025

Why using packaging when redis-py defines VERSION and __version__ at package level?

https://github.com/redis/redis-py/blob/5977e387760cb30936629919d5dc76a27652c790/redis/__init__.py#L48-L49

And at least from v4.5.2 (which is the current minimum target of kombu):

https://github.com/redis/redis-py/blob/318b114f4da9846a2a7c150e1fb702e9bebd9fdf/redis/__init__.py#L50-L59

This could have just been

_REDIS_GET_CONNECTION_WITHOUT_ARGS = redis.VERSION >= (5, 3)

No extra imports required, no extra dependencies.

@woutdenolf
Copy link
Copy Markdown
Contributor Author

The motivation for this is described in this comment: #2294 (comment).

@auvipy
Copy link
Copy Markdown
Member

auvipy commented Jun 4, 2025

Why using packaging when redis-py defines VERSION and __version__ at package level?

https://github.com/redis/redis-py/blob/5977e387760cb30936629919d5dc76a27652c790/redis/__init__.py#L48-L49

And at least from v4.5.2 (which is the current minimum target of kombu):

https://github.com/redis/redis-py/blob/318b114f4da9846a2a7c150e1fb702e9bebd9fdf/redis/__init__.py#L50-L59

This could have just been

_REDIS_GET_CONNECTION_WITHOUT_ARGS = redis.VERSION >= (5, 3)

No extra imports required, no extra dependencies.

I agree with the reasoning of @sevdog here

@sevdog
Copy link
Copy Markdown

sevdog commented Jun 4, 2025

The motivation for this is described in this comment: #2294 (comment).

That would be a good solution when you have a generic dependency and you must handle it in a generic way. Since the dependency here is known and because this aims to fix a derecation warning adding a new dependency and two imports to address this looks a bit of overengineering.

@auvipy
Copy link
Copy Markdown
Member

auvipy commented Jun 4, 2025

I will open a pr to fix this

joeriddles pushed a commit to joeriddles/kombu that referenced this pull request Oct 17, 2025
…ccept arguments (celery#2294)

* redis.connection.ConnectionPool API change: get_connection does not accept arguments


---------

Co-authored-by: Asif Saif Uddin <auvipy@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

redis.connection.ConnectionPool: get_connection('_') is deprecated since 5.3.0

3 participants