commit 575f1b6685f6b8be3b3801ff662a4d6d89db43c4 Author: Damian Johnson atagar@torproject.org Date: Thu Aug 16 10:33:47 2018 -0700

Drop unsigned from Size class

Actually, it's already in our pydoc that these are all unsigned. Maybe we'll add a flag like this in the future if that changes but presently it doesn't provide any value.

That said, I *do* like the idea of providing a better error message. Keeping that, and improving our performance a bit by only performing these checks if packing fails (this class is heavily used in IO so making this a tad extra performant is useful). --- stem/client/datatype.py | 16 +++++++++------- test/unit/client/size.py | 9 +-------- test/unit/control/controller.py | 1 - 3 files changed, 10 insertions(+), 16 deletions(-)

diff --git a/stem/client/datatype.py b/stem/client/datatype.py index 1c7aec77..43bdfe9d 100644 --- a/stem/client/datatype.py +++ b/stem/client/datatype.py @@ -349,19 +349,21 @@ class Size(Field): self.name = name self.size = size self.format = pack_format - self.unsigned = pack_format.isupper()

@staticmethod def pop(packed): raise NotImplementedError("Use our constant's unpack() and pop() instead")

def pack(self, content): - if not stem.util._is_int(content): - raise ValueError('Size.pack encodes an integer, but was a %s' % type(content).__name__) - if content < 0 and self.unsigned: - raise ValueError('A %s field cannot pack negative values, but %i was tried' % (self.name, content)) - - packed = struct.pack(self.format, content) + try: + packed = struct.pack(self.format, content) + except struct.error: + if not stem.util._is_int(content): + raise ValueError('Size.pack encodes an integer, but was a %s' % type(content).__name__) + elif content < 0: + raise ValueError('Packed values must be positive (attempted to pack %i as a %s)' % (content, self.name)) + else: + raise # some other struct exception

if self.size != len(packed): raise ValueError('%s is the wrong size for a %s field' % (repr(packed), self.name)) diff --git a/test/unit/client/size.py b/test/unit/client/size.py index 3d7d796f..733db0f4 100644 --- a/test/unit/client/size.py +++ b/test/unit/client/size.py @@ -7,22 +7,17 @@ import unittest

from stem.client.datatype import Size

-SIGNED_CHAR = Size('SIGNED_CHAR', 1, '!b') -

class TestSize(unittest.TestCase): def test_attributes(self): self.assertEqual('CHAR', Size.CHAR.name) self.assertEqual('!B', Size.CHAR.format) - self.assertEqual(True, Size.CHAR.unsigned)

self.assertEqual(1, Size.CHAR.size) self.assertEqual(2, Size.SHORT.size) self.assertEqual(4, Size.LONG.size) self.assertEqual(8, Size.LONG_LONG.size)

- self.assertEqual(False, SIGNED_CHAR.unsigned) - def test_pack(self): self.assertEqual(b'\x12', Size.CHAR.pack(18)) self.assertEqual(b'\x00\x12', Size.SHORT.pack(18)) @@ -31,13 +26,11 @@ class TestSize(unittest.TestCase):

self.assertRaisesWith(ValueError, 'Size.pack encodes an integer, but was a str', Size.CHAR.pack, 'hi')

- self.assertRaisesWith(ValueError, 'A CHAR field cannot pack negative values, but -1 was tried', Size.CHAR.pack, -1) + self.assertRaisesWith(ValueError, 'Packed values must be positive (attempted to pack -1 as a CHAR)', Size.CHAR.pack, -1)

bad_size = Size('BAD_SIZE', 1, '!H') self.assertRaisesRegexp(ValueError, re.escape("'\x00\x12' is the wrong size for a BAD_SIZE field"), bad_size.pack, 18)

- self.assertEqual(b'\xFF', SIGNED_CHAR.pack(-1)) - def test_unpack(self): self.assertEqual(18, Size.CHAR.unpack(b'\x12')) self.assertEqual(18, Size.SHORT.unpack(b'\x00\x12')) diff --git a/test/unit/control/controller.py b/test/unit/control/controller.py index 7ee2ab70..ba55208a 100644 --- a/test/unit/control/controller.py +++ b/test/unit/control/controller.py @@ -5,7 +5,6 @@ integ tests, but a few bits lend themselves to unit testing.

import datetime import io -import time import unittest

import stem.descriptor.router_status_entry