-
-
Notifications
You must be signed in to change notification settings - Fork 12
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
Conversation
Codecov ReportAttention: Patch coverage is
❗ 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. |
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.
👍 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 |
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.
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.
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.
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?
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.
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).
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.
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.
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.
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.
this PR fix #118