Skip to content

Remove a few unused imports #1188

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

Merged
merged 3 commits into from
Aug 30, 2017
Merged

Remove a few unused imports #1188

merged 3 commits into from
Aug 30, 2017

Conversation

jameslamb
Copy link
Contributor

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.

Copy link
Contributor

@jeffwidman jeffwidman left a 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

@@ -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
Copy link
Contributor

@jeffwidman jeffwidman Aug 30, 2017

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():

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 using socket.socketpair() in this file.

Copy link
Contributor Author

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

@@ -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

Copy link
Contributor

@jeffwidman jeffwidman Aug 30, 2017

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?

Copy link
Contributor Author

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

@jeffwidman jeffwidman merged commit 24bf504 into dpkp:master Aug 30, 2017
@jeffwidman
Copy link
Contributor

Thanks!

88manpreet pushed a commit to Yelp/kafka-python that referenced this pull request Jul 16, 2018
* Removed a few unused imports

* Added note on socketpair monkey-path
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.

2 participants