Better type errors

[mdaniel]

In 0.9.2 (and master), if one passes a group to SimpleConsumer that is a unicode string, it bombs with the terribly unhelpful message below. This PR merely makes the error more helpful by providing a message describing what should have happened and what type was provided. With it in place, the fix would be obvious.

Traceback (most recent call last):
  File "sniff_topic.py", line 52, in <module>
    main(sys.argv)                          
  File "sniff_topic.py", line 38, in main   
    cons = SimpleConsumer(client, group, topic)
  File "/home/ubuntu/venv/local/lib/python2.7/site-packages/kafka/consumer.py", line 243, in __init__
    auto_commit_every_t=auto_commit_every_t)
  File "/home/ubuntu/venv/local/lib/python2.7/site-packages/kafka/consumer.py", line 115, in __init__
    fail_on_error=False)
  File "/home/ubuntu/venv/local/lib/python2.7/site-packages/kafka/client.py", line 384, in send_offset_fetch_request
    resps = self._send_broker_aware_request(payloads, encoder, decoder)
  File "/home/ubuntu/venv/local/lib/python2.7/site-packages/kafka/client.py", line 151, in _send_broker_aware_request
    correlation_id=requestId, payloads=payloads)
  File "/home/ubuntu/venv/local/lib/python2.7/site-packages/kafka/protocol.py", line 482, in encode_offset_fetch_request
    message += write_short_string(group)
  File "/home/ubuntu/venv/local/lib/python2.7/site-packages/kafka/util.py", line 23, in write_short_string
    return struct.pack('>h%ds' % len(s), len(s), s)
struct.error: argument for 's' must be a string

p.s. If you have not yet tried PyCharm you don't know what you're missing. I'm surprised that unittest2 allows such things, but the test.test_util.UtilTest class redeclared the same method name over and over.

[dpkp]

what does this comment mean?

[dpkp]

vim for me :) I have been meaning to take a linter to the code for a while, which should catch issues like this, too:

$ pyflakes test_util.py
test_util.py:1: 'os' imported but unused
test_util.py:2: 'random' imported but unused
test_util.py:67: redefinition of unused 'test_read_int_string__insufficient_data' from line 36
test_util.py:71: redefinition of unused 'test_relative_unpack' from line 9
test_util.py:77: redefinition of unused 'test_relative_unpack' from line 71

[dpkp]

i guess the better question is what does this mean...

[mdaniel]

We had people on my team who swore by vim as well, because one can manipulate text amazingly well in vim. The trouble is that a Python programmer is not manipulating text, they are paid to manipulate Python code. PyCharm works with code, vim works with text. But it's your project, you can do as you'd like.

The # noinspection PyAttributeOutsideInit is because of a warning that the class spontaneously acquired a timer attribute which wasn't mentioned elsewhere in the class; it is unclear what that statement was trying to do, but because I couldn't prove it was wrong, I just asked PyCharm to not warn me (or follow-on coders) about it.

[mumrah]

That's a funny thing about Python objects, you can set any attribute you'd like without much complaint. As for this particular assignment, if effective does nothing but create an attribute in the object's __dict__ with a value of None, so it's pretty low impact.

I do believe it is vestigial code though, I don't see any other references to "timer" anywhere (like you say)

[mumrah]

Oh and another note: you should never ever mix source code with editor preferences. I mean, really!

[wizzat]

I think it's pretty poor form to start an editor flame war in a code review, or to assert that text editing is somehow not also editing code. One could just as easily assert that vim edits code just as well if not better than Pycharm without crippling your ability to edit the text the code is composed of.

;-)

-Mark

On Aug 22, 2014, at 17:20, Matthew Daniel [email protected] wrote:

In kafka/util.py:

@@ -117,4 +123,5 @@ def stop(self):

     self.active.set()
     self.thread.join(self.t + 1)

•    # noinspection PyAttributeOutsideInit
   self.timer = None
  

  We had people on my team who swore by vim as well, because one can manipulate text amazingly well in vim. The trouble is that a Python programmer is not manipulating text, they are paid to manipulate Python code. PyCharm works with code, vim works with text. But it's your project, you can do as you'd like.

The # noinspection PyAttributeOutsideInit is because of a warning that the class spontaneously acquired a timer attribute which wasn't mentioned elsewhere in the class; it is unclear what that statement was trying to do, but because I couldn't prove it was wrong, I just asked PyCharm to not warn me (or follow-on coders) about it.

—
Reply to this email directly or view it on GitHub.

[dpkp]

agree with all of that, but this PR does fix some good stuff. re editors, meh. it's typically just a wrapper on one of the standard linting tools. I opened #205 to get that into our test pipeline, which I think would address the underlying issue here.

[dpkp]

tests pass -- merging!