-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Remove a few unused imports #1188
Conversation
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 putting this together!
Everything looks good except the one comment
kafka/client_async.py
Outdated
@@ -28,8 +28,6 @@ | |||
from .metrics.stats import Avg, Count, Rate | |||
from .metrics.stats.rate import TimeUnit | |||
from .protocol.metadata import MetadataRequest | |||
from .protocol.produce import ProduceRequest | |||
from .vendor import socketpair |
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 one should be left in, as it monkey-patches socket.socketpair()
:
kafka-python/kafka/vendor/socketpair.py
Line 58 in ba7afd9
socket.socketpair = socketpair |
Rather than removing, can you add a comment right above it saying something like:
Although this looks unused, it actually monkey-patches
socket.socketpair()
and should be left in as long as we're usingsocket.socketpair()
in this file.
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.
Got it! Yep, I can make that change
kafka/client_async.py
Outdated
@@ -28,7 +28,9 @@ | |||
from .metrics.stats import Avg, Count, Rate | |||
from .metrics.stats.rate import TimeUnit | |||
from .protocol.metadata import MetadataRequest | |||
from .protocol.produce import ProduceRequest | |||
|
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.
minor nit: Can you remove this empty line, per PEP-8 style guide?
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.
ha hey if more people nitpicked about PEP-8 stuff, the world would be a happier and safer place!
I am on it. One moment
Thanks! |
* Removed a few unused imports * Added note on socketpair monkey-path
Thank you for creating and maintaining this excellent package! I was reading through the code today just trying to get a better sense of the internals, and found a few unused imports. Please consider this PR to remove them.