[tor-commits] [stem/master] Drop unsigned from Size class

[atagar at torproject.org]

commit 575f1b6685f6b8be3b3801ff662a4d6d89db43c4
Author: Damian Johnson <atagar at 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