-
Notifications
You must be signed in to change notification settings - Fork 234
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
add typing to aiokafka/conn.py #1021
base: master
Are you sure you want to change the base?
Conversation
return self._port | ||
|
||
def send(self, request, expect_response=True): | ||
@overload | ||
def send(self, request: Request[ResponseT]) -> Coroutine[None, None, ResponseT]: ... |
Check notice
Code scanning / CodeQL
Statement has no effect Note
@overload | ||
def send( | ||
self, request: Request[ResponseT], expect_response: Literal[False] | ||
) -> Coroutine[None, None, None]: ... |
Check notice
Code scanning / CodeQL
Statement has no effect Note
@overload | ||
def _send_sasl_token( | ||
self, payload: bytes, expect_response: Literal[False] | ||
) -> Coroutine[None, None, None]: ... |
Check notice
Code scanning / CodeQL
Statement has no effect Note
@overload | ||
def _send_sasl_token( | ||
self, payload: bytes, expect_response: bool | ||
) -> Union[Coroutine[None, None, None], Coroutine[None, None, bytes]]: ... |
Check notice
Code scanning / CodeQL
Statement has no effect Note
class CloseReason: | ||
class Packet(NamedTuple): | ||
correlation_id: int | ||
request: Request[Response] |
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.
Request is not generic
aiokafka/conn.py
Outdated
if correlation_id is None: # Is a SASL packet, just pass it though | ||
if not fut.done(): | ||
fut.set_result(resp) | ||
if packet.correlation_id is None: # Is a SASL packet, just pass it though |
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.
There is not need to define meaningless fields in SaslPacket
, if we use isinstance
here
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.
class SaslPacket(NamedTuple):
fut: asyncio.Future
without correation_id and request?
@overload | ||
def _send_sasl_token( | ||
self, payload: bytes, expect_response: Literal[True] | ||
) -> Coroutine[None, None, bytes]: ... |
Check notice
Code scanning / CodeQL
Statement has no effect Note
self, payload: bytes, expect_response: Literal[True] | ||
) -> Coroutine[None, None, bytes]: ... | ||
@overload | ||
def _send_sasl_token(self, payload: bytes) -> Coroutine[None, None, bytes]: ... |
Check notice
Code scanning / CodeQL
Statement has no effect Note
resp = cast( | ||
Union[SaslAuthenticateResponse_v0, SaslAuthenticateResponse_v1], | ||
resp, | ||
) |
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's required for mypy (but ok for pyright), problem only with union types (I think problem with https://github.com/python/mypy/labels/topic-join-v-union)
return ScramAuthenticator( | ||
loop=self._loop, | ||
sasl_plain_password=self._sasl_plain_password, | ||
sasl_plain_username=self._sasl_plain_username, | ||
sasl_mechanism=self._sasl_mechanism, | ||
sasl_mechanism=self._sasl_mechanism, # type: ignore[arg-type] |
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's required for mypy (but ok for pyright)
@@ -418,12 +507,13 @@ def _on_read_task_error(cls, self_ref, read_task): | |||
self.close(reason=CloseReason.CONNECTION_BROKEN, exc=exc) | |||
|
|||
@staticmethod | |||
def _idle_check(self_ref): | |||
def _idle_check(self_ref: weakref.ReferenceType[AIOKafkaConnection]) -> None: |
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 should be Self, but Self "isn't allowed in this context"
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1021 +/- ##
==========================================
- Coverage 95.02% 95.00% -0.02%
==========================================
Files 114 114
Lines 16970 17069 +99
Branches 2771 2779 +8
==========================================
+ Hits 16125 16216 +91
- Misses 497 498 +1
- Partials 348 355 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Changes
Fixes #
Checklist
CHANGES
folder<issue_id>.<type>
(e.g.588.bugfix
)issue_id
change it to the pr id after creating the PR.feature
: Signifying a new feature..bugfix
: Signifying a bug fix..doc
: Signifying a documentation improvement..removal
: Signifying a deprecation or removal of public API..misc
: A ticket has been closed, but it is not of interest to users.Fix issue with non-ascii contents in doctest text files.