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

add typing to aiokafka/protocol/* #999

Merged
merged 16 commits into from
Apr 21, 2024

Conversation

dimastbk
Copy link
Contributor

Changes

partial fixes #980

Add typing to all modules in protocol package.

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> (e.g. 588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the PR
    • ensure type is one of the following:
      • .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.
    • Make sure to use full sentences with correct case and punctuation, for example: Fix issue with non-ascii contents in doctest text files.

aiokafka/protocol/admin.py Show resolved Hide resolved
aiokafka/protocol/api.py Outdated Show resolved Hide resolved
aiokafka/protocol/api.py Show resolved Hide resolved
aiokafka/protocol/fetch.py Show resolved Hide resolved
aiokafka/protocol/message.py Show resolved Hide resolved
aiokafka/protocol/admin.py Outdated Show resolved Hide resolved
aiokafka/protocol/metadata.py Outdated Show resolved Hide resolved
aiokafka/protocol/produce.py Outdated Show resolved Hide resolved
@dimastbk
Copy link
Contributor Author

Maybe rewrite all schemas using dataclasses? If it is not a problem for performance.

aiokafka/protocol/api.py Outdated Show resolved Hide resolved
aiokafka/protocol/message.py Show resolved Hide resolved
aiokafka/protocol/message.py Outdated Show resolved Hide resolved
aiokafka/protocol/message.py Outdated Show resolved Hide resolved
aiokafka/protocol/message.py Show resolved Hide resolved
aiokafka/protocol/metadata.py Outdated Show resolved Hide resolved
aiokafka/protocol/message.py Outdated Show resolved Hide resolved
aiokafka/protocol/struct.py Outdated Show resolved Hide resolved
aiokafka/protocol/message.py Show resolved Hide resolved
@classmethod
@abc.abstractmethod
def encode(cls, value): ...
def encode(cls, value: T) -> bytes: ...

Check notice

Code scanning / CodeQL

Statement has no effect Note

This statement has no effect.

@classmethod
@abc.abstractmethod
def decode(cls, data): ...
def decode(cls, data: BytesIO) -> T: ...

Check notice

Code scanning / CodeQL

Statement has no effect Note

This statement has no effect.
magic: Literal[0],
attributes: int,
crc: int,
) -> None: ...

Check notice

Code scanning / CodeQL

Statement has no effect Note

This statement has no effect.
attributes: int,
crc: int,
timestamp: int,
) -> None: ...

Check notice

Code scanning / CodeQL

Statement has no effect Note

This statement has no effect.
array_of: ValueT

@overload
def __init__(self, array_of_0: ValueT): ...

Check notice

Code scanning / CodeQL

Statement has no effect Note

This statement has no effect.
@overload
def __init__(
self, array_of_0: Tuple[str, ValueT], *array_of: Tuple[str, ValueT]
): ...

Check notice

Code scanning / CodeQL

Statement has no effect Note

This statement has no effect.
@@ -23,27 +25,29 @@
)
)

def encode(self):
def encode(self) -> bytes:

Check notice

Code scanning / CodeQL

Mismatch between signature and use of an overridden method Note

Overridden method signature does not match
call
, where it is passed too many arguments. Overriding method
method Message.encode
matches the call.
Overridden method signature does not match
call
, where it is passed too many arguments. Overriding method
method Message.encode
matches the call.
Overridden method signature does not match
call
, where it is passed an argument named 'recalc_crc'. Overriding method
method Message.encode
matches the call.
Overridden method signature does not match
call
, where it is passed an argument named 'recalc_crc'. Overriding method
method Message.encode
matches the call.
Copy link

codecov bot commented Apr 20, 2024

Codecov Report

Attention: Patch coverage is 89.71963% with 22 lines in your changes are missing coverage. Please review.

Project coverage is 95.38%. Comparing base (1855cde) to head (afe0439).
Report is 5 commits behind head on master.

Files Patch % Lines
aiokafka/protocol/message.py 73.33% 9 Missing and 3 partials ⚠️
aiokafka/protocol/types.py 93.40% 3 Missing and 3 partials ⚠️
aiokafka/protocol/abstract.py 71.42% 0 Missing and 2 partials ⚠️
aiokafka/protocol/struct.py 84.61% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #999      +/-   ##
==========================================
- Coverage   95.44%   95.38%   -0.07%     
==========================================
  Files         112      112              
  Lines       16702    16738      +36     
  Branches     2672     2691      +19     
==========================================
+ Hits        15942    15966      +24     
- Misses        479      485       +6     
- Partials      281      287       +6     
Flag Coverage Δ
cext 92.11% <89.71%> (-0.06%) ⬇️
integration 95.03% <89.71%> (-0.07%) ⬇️
purepy 94.84% <89.71%> (-0.08%) ⬇️
unit 52.69% <87.85%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@ods ods left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Dmitry!
Thank you very much, you did a huge piece of work! And sorry for delay with review. I hope we'll finish it very soon.

aiokafka/protocol/message.py Show resolved Hide resolved
aiokafka/protocol/message.py Outdated Show resolved Hide resolved
aiokafka/protocol/metadata.py Outdated Show resolved Hide resolved
aiokafka/protocol/message.py Outdated Show resolved Hide resolved
aiokafka/protocol/message.py Show resolved Hide resolved
aiokafka/protocol/message.py Outdated Show resolved Hide resolved
aiokafka/protocol/types.py Outdated Show resolved Hide resolved
@dimastbk dimastbk requested a review from ods April 21, 2024 17:02
@ods ods merged commit 1862620 into aio-libs:master Apr 21, 2024
28 of 31 checks passed
@dimastbk
Copy link
Contributor Author

Thanks for the review, sorry for 64 comments

@ods
Copy link
Collaborator

ods commented Apr 21, 2024

Hmm, we forgot about test_protocol.py and test_protocol_object_conversion.py. They are important, because they allow us to verify how types in protocol module actually work. Unfortunately, they don't. We need to do something with attributes from schema, otherwise everything will in # type: ignore.

@ods ods mentioned this pull request Jul 6, 2024
4 tasks
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.

Proposal to Add Type Hints
2 participants