-
-
Notifications
You must be signed in to change notification settings - Fork 930
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
feature: Adding type annotations to Connections #1612
base: main
Are you sure you want to change the base?
feature: Adding type annotations to Connections #1612
Conversation
This pull request introduces 1 alert when merging 58804fc into 22b5596 - view on LGTM.com new alerts:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kombu/pools.py:7:1: F401 'typing.Optional' imported but unused
return connection.Pool(limit=limit) | ||
|
||
def __getitem__(self, connection: Connection) -> ConnectionPool: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to know, what this dunder methods are going to achieve?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have made it very clear
Code editors such as vscode, pycharm, etc. cannot provide intelligent type inference for this connections[connection].acquire(block=True) code, so I would like to add typing hint to achieve this (intelligent type inference)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When your mouse hovers over a conn, vscode will tell you that it is the type of this conn is Any, which is very stupid.
When hovering over a conn, I wish vscode would tell me what the explicit type is, instead of a stupid and damned Any
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I added these typing hint which will solve this problem very well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pull request introduces 2 alerts when merging c6d006b into 22b5596 - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging d1fdaee into 22b5596 - view on LGTM.com new alerts:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kombu/pools.py:7:1: F401 'typing.Optional' imported but unused
kombu/pools.py:82:80: E501 line too long (80 > 79 characters)
kombu/connection.py:11:1: F401 'typing.Optional' imported but unused
kombu/connection.py:11:1: F401 'typing.Union' imported but unused
kombu/connection.py:1042:80: E501 line too long (93 > 79 characters)
ERROR: InvocationError for command /home/runner/work/kombu/kombu/.tox/flake8/bin/flake8 -j2 kombu t (exited with code 1)
@@ -1039,6 +1039,9 @@ def prepare(self, resource): | |||
resource._debug('acquired') | |||
return resource | |||
|
|||
def acquire(self, block: bool = False, timeout: int | float | None = None) -> Connection: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this method newly introduced?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not new, ConnectionPool inherits from Resource
Resource has an acquire method, but since Resource is an abstract class (abstracting the connection as a resource), this -> Connection
type annotation should not be added to the acquire method of Resource, so I added an acquire method to ConnectionPool and added the I added an acquire method to the ConnectionPool and added the -> Connection
type annotation to the ConnectionPool to make it semantic
kombu/connection.py
class ConnectionPool(Resource):
"""Pool of connections."""
def acquire(self, block: bool = False, timeout: Union[int, float, None] = None) -> Connection:
return super().acquire(block, timeout)
kombu/resource.py
class Resource:
"""Pool of resources."""
def acquire(self, block=False, timeout=None):
"""Acquire resource.
Arguments:
block (bool): If the limit is exceeded,
then block until there is an available item.
timeout (float): Timeout to wait
if ``block`` is true. Default is :const:`None` (forever).
Raises:
LimitExceeded: if block is false and the limit has been exceeded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be cool to have an entry on the pyproject.toml
associated with the annotation. 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a better way than re-defining existing methods just to provide typing information for them?
I'm not satisfied with the current approach.
Code editors such as vscode, pycharm, etc. cannot provide intelligent type inference for this
connections[connection].acquire(block=True)
code, so I would like to add typing hint to achieve this (intelligent type inference)