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 type hints & raises error when consumer is closed #122

Merged
merged 4 commits into from
Jul 10, 2024

Conversation

alm0ra
Copy link
Owner

@alm0ra alm0ra commented Jul 10, 2024

this PR fix #118

@alm0ra alm0ra self-assigned this Jul 10, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jul 10, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 97.29730% with 1 line in your changes missing coverage. Please review.

Project coverage is 98.12%. Comparing base (c0b2cfe) to head (8a4cde9).

Files Patch % Lines
mockafka/aiokafka/aiokafka_consumer.py 94.44% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #122      +/-   ##
==========================================
- Coverage   98.16%   98.12%   -0.04%     
==========================================
  Files          35       35              
  Lines        1470     1497      +27     
==========================================
+ Hits         1443     1469      +26     
- Misses         27       28       +1     

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

@alm0ra alm0ra merged commit 7b34aa8 into main Jul 10, 2024
10 checks passed
Copy link
Collaborator

@PeterJCLaw PeterJCLaw left a comment

Choose a reason for hiding this comment

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

👍 Some small suggestions/questions inline.

@@ -6,9 +6,10 @@
import re
import warnings
from collections.abc import Iterable, Iterator
from typing import Any
from typing import Any, Optional
Copy link
Collaborator

Choose a reason for hiding this comment

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

For my understanding, perhaps this is a style thing -- is there a reason to use Optional here over the newer and (mostly) clearer pipe syntax for unions? All the versions of Python which this library supports support future annotations, which I note is already imported almost universally, so the pipe syntax should be supported just fine.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, it's mostly about styling, and you did your typing correctly. However, in mockafka, I primarily used the typing module, such as Optional or Union, since mockafka supports Python 3.8 and above.

But I'm not sure, is it really newer to use Pipe instead of Union?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, the pipe syntax was introduced by PEP 604 which targetted the 3.10 standard library. (typing.Union has been around since the very early days of the typing module) It's usable in older versions of Python due to the from __future__ import annotations causing the interpreter not to evaluate the annotations.

The pipe syntax is just a syntactic sugar though -- under the covers the value returned is still an instance of typing.Union.

I find that the pipe syntax is (mostly) much clearer and it's certainly more succinct. It also has the benefit that unions involving None don't look like special cases -- they're just unions (as they have always been).

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks for the information, I understand now.

I've sent you an invitation to become a maintainer of mockafka. If you're interested, we could create a new organization and move the project there.

I don't have access to your email, but if you'd like to discuss this further, please feel free to send me an email.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, I'm happy to contribute a little, though I probably don't have time to take on a significant role.

Up to you whether to create an org or keep the repo where it is; I'm happy either way.

For background context: I have a fair amount of experience with Python, though my experience with Kafka is limited to peripheral interactions over the past few weeks.

mockafka/aiokafka/aiokafka_consumer.py Show resolved Hide resolved
mockafka/aiokafka/aiokafka_consumer.py Show resolved Hide resolved
tests/test_aiokafka/test_aiokafka_consumer.py Show resolved Hide resolved
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.

FakeAIOKafkaConsumer never raises error about being closed
3 participants