Skip to content

Better type errors #204

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.

Sign up for GitHub

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 5 commits into from
Aug 22, 2014
Merged

Better type errors #204

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
08c2945
Correctly declare unittest2 dependency
mdaniel Aug 22, 2014
b4bdd8e
PEP8 fixes
mdaniel Aug 22, 2014
a27e521
Failing test for write_int and write_short
mdaniel Aug 22, 2014
bb3827e
PEP8 fixes
mdaniel Aug 22, 2014
6d6dc17
Fix write_int and write_short type validation
mdaniel Aug 22, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion kafka/util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,22 @@


def write_int_string(s):
if s is not None and not isinstance(s, str):
raise TypeError('Expected "%s" to be str\n'
'data=%s' % (type(s), repr(s)))
if s is None:
return struct.pack('>i', -1)
else:
return struct.pack('>i%ds' % len(s), len(s), s)


def write_short_string(s):
if s is not None and not isinstance(s, str):
raise TypeError('Expected "%s" to be str\n'
'data=%s' % (type(s), repr(s)))
if s is None:
return struct.pack('>h', -1)
elif len(s) > 32767 and sys.version < (2,7):
elif len(s) > 32767 and sys.version < (2, 7):
# Python 2.6 issues a deprecation warning instead of a struct error
raise struct.error(len(s))
else:
Expand Down Expand Up @@ -117,4 +123,5 @@ def stop(self):

self.active.set()
self.thread.join(self.t + 1)
# noinspection PyAttributeOutsideInit
Copy link
Owner

Choose a reason for hiding this comment

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

what does this comment mean?

self.timer = None
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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.

6 changes: 4 additions & 2 deletions setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
with open('VERSION', 'r') as v:
__version__ = v.read().rstrip()


class Tox(Command):

user_options = []
Expand All @@ -15,7 +16,8 @@ def initialize_options(self):
def finalize_options(self):
pass

def run(self):
@classmethod
def run(cls):
import tox
sys.exit(tox.cmdline([]))

Expand All @@ -24,7 +26,7 @@ def run(self):
name="kafka-python",
version=__version__,

tests_require=["tox", "mock"],
tests_require=["tox", "mock", "unittest2"],
cmdclass={"test": Tox},

packages=["kafka"],
Expand Down
39 changes: 27 additions & 12 deletions test/test_util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import os
import random
# -*- coding: utf-8 -*-
import struct
import unittest2
import kafka.util
import kafka.common


class UtilTest(unittest2.TestCase):
@unittest2.skip("Unwritten")
def test_relative_unpack(self):
Expand All @@ -16,6 +16,14 @@ def test_write_int_string(self):
'\x00\x00\x00\x0bsome string'
)

def test_write_int_string__unicode(self):
with self.assertRaises(TypeError) as cm:
kafka.util.write_int_string(u'unicode')
#: :type: TypeError
te = cm.exception
self.assertIn('unicode', te.message)
self.assertIn('to be str', te.message)

def test_write_int_string__empty(self):
self.assertEqual(
kafka.util.write_int_string(''),
Expand Down Expand Up @@ -43,6 +51,14 @@ def test_write_short_string(self):
'\x00\x0bsome string'
)

def test_write_short_string__unicode(self):
with self.assertRaises(TypeError) as cm:
kafka.util.write_short_string(u'hello')
#: :type: TypeError
te = cm.exception
self.assertIn('unicode', te.message)
self.assertIn('to be str', te.message)

def test_write_short_string__empty(self):
self.assertEqual(
kafka.util.write_short_string(''),
Expand All @@ -64,21 +80,20 @@ def test_read_short_string(self):
self.assertEqual(kafka.util.read_short_string('\x00\x00', 0), ('', 2))
self.assertEqual(kafka.util.read_short_string('\x00\x0bsome string', 0), ('some string', 13))

def test_read_int_string__insufficient_data(self):
def test_read_int_string__insufficient_data2(self):
with self.assertRaises(kafka.common.BufferUnderflowError):
kafka.util.read_int_string('\x00\x021', 0)

def test_relative_unpack(self):
def test_relative_unpack2(self):
self.assertEqual(
kafka.util.relative_unpack('>hh', '\x00\x01\x00\x00\x02', 0),
((1, 0), 4)
)

def test_relative_unpack(self):
def test_relative_unpack3(self):
with self.assertRaises(kafka.common.BufferUnderflowError):
kafka.util.relative_unpack('>hh', '\x00', 0)


def test_group_by_topic_and_partition(self):
t = kafka.common.TopicAndPartition

Expand All @@ -91,12 +106,12 @@ def test_group_by_topic_and_partition(self):
]

self.assertEqual(kafka.util.group_by_topic_and_partition(l), {
"a" : {
1 : t("a", 1),
2 : t("a", 2),
3 : t("a", 3),
"a": {
1: t("a", 1),
2: t("a", 2),
3: t("a", 3),
},
"b" : {
3 : t("b", 3),
"b": {
3: t("b", 3),
}
})