configglue team mailing list archive

[Ricardo Kirkner <ricardo.kirkner@xxxxxxxxxxxxx>]

Ricardo Kirkner has proposed merging lp:~ricardokirkner/configglue/better-hash into lp:configglue.

Commit message:
use stricter hashing functions to avoid potential pitfall

Requested reviews:
  Configglue developers (configglue)

For more details, see:
https://code.launchpad.net/~ricardokirkner/configglue/better-hash/+merge/211843
-- 
https://code.launchpad.net/~ricardokirkner/configglue/better-hash/+merge/211843
Your team Configglue developers is requested to review the proposed merge of lp:~ricardokirkner/configglue/better-hash into lp:configglue.

=== modified file 'configglue/schema.py'
--- configglue/schema.py	2013-07-11 17:01:58 +0000
+++ configglue/schema.py	2014-03-19 23:28:30 +0000
@@ -158,7 +158,8 @@
         return not self.__eq__(other)
 
     def __hash__(self):
-        return id(self)
+        attrs = ['_sections', 'includes']
+        return hash(hash(getattr(self, attr)) for attr in attrs)
 
     def is_valid(self):
         """Return whether the schema has a valid structure."""
@@ -227,7 +228,9 @@
         return not self.__eq__(other)
 
     def __hash__(self):
-        return id(self)
+        return hash(
+            hash(self.name) + hash(hash(option) for option in self.options())
+        )
 
     def __repr__(self):
         if self.name:
@@ -324,7 +327,11 @@
         return not self.__eq__(other)
 
     def __hash__(self):
-        return id(self)
+        attrs = [
+            'name', 'short_name', 'raw', 'fatal', 'default', 'help', 'action',
+            'section',
+        ]
+        return hash(hash(getattr(self, attr)) for attr in attrs)
 
     def __repr__(self):
         extra = ' raw' if self.raw else ''
@@ -443,7 +450,10 @@
         return equal
 
     def __hash__(self):
-        return id(self)
+        attrs = [
+            'item', 'require_parser', 'raw', 'remove_duplicates',
+        ]
+        return hash(hash(getattr(self, attr)) for attr in attrs)
 
     def _get_default(self):
         return []
@@ -515,7 +525,8 @@
         return equal
 
     def __hash__(self):
-        return id(self)
+        attrs = ['null']
+        return hash(hash(getattr(self, attr)) for attr in attrs)
 
     def _get_default(self):
         return '' if not self.null else None
@@ -569,7 +580,8 @@
         return equal
 
     def __hash__(self):
-        return id(self)
+        attrs = ['length']
+        return hash(hash(getattr(self, attr)) for attr in attrs)
 
     def _get_default(self):
         return ()
@@ -640,7 +652,8 @@
         return equal
 
     def __hash__(self):
-        return id(self)
+        attrs = ['spec', 'strict', 'item']
+        return hash(hash(getattr(self, attr)) for attr in attrs)
 
     def _get_default(self):
         default = {}

=== modified file 'configglue/tests/test_schema.py'
--- configglue/tests/test_schema.py	2013-07-11 17:01:58 +0000
+++ configglue/tests/test_schema.py	2014-03-19 23:28:30 +0000
@@ -134,6 +134,15 @@
         self.assertEqual(MySchema(), MySchema())
         self.assertNotEqual(MySchema(), OtherSchema())
 
+    def test_same_hash_if_equal(self):
+        class MySchema(Schema):
+            foo = IntOption()
+
+        my_schema = MySchema()
+        other_schema = MySchema()
+        self.assertEqual(my_schema, other_schema)
+        self.assertEqual(hash(my_schema), hash(other_schema))
+
 
 class TestSchemaHelpers(unittest.TestCase):
     def test_get_config_objects(self):
@@ -194,6 +203,13 @@
         del opt2.name
         self.assertNotEqual(opt1, opt2)
 
+    def test_same_hash_if_equal(self):
+        opt1 = IntOption(name='opt1')
+        opt2 = IntOption(name='opt1')
+
+        self.assertEqual(opt1, opt2)
+        self.assertEqual(hash(opt1), hash(opt2))
+
     def test_validate(self):
         """Test Option default validate behaviour."""
         opt = self.cls()
@@ -379,6 +395,13 @@
         self.assertEqual(option1, option1)
         self.assertNotEqual(option1, option2)
 
+    def test_same_hash_if_equal(self):
+        option1 = StringOption()
+        option2 = StringOption()
+
+        self.assertEqual(option1, option2)
+        self.assertEqual(hash(option1), hash(option2))
+
 
 class TestIntOption(unittest.TestCase):
     cls = IntOption
@@ -651,6 +674,13 @@
         self.assertNotEqual(option1, option6)
         self.assertNotEqual(option1, option7)
 
+    def test_same_hash_if_equal(self):
+        option1 = ListOption()
+        option2 = ListOption(item=StringOption())
+
+        self.assertEqual(option1, option2)
+        self.assertEqual(hash(option1), hash(option2))
+
     def test_to_string_when_json(self):
         option = ListOption()
         result = option.to_string(['1', '2', '3'])
@@ -739,6 +769,13 @@
         self.assertEqual(option1, option1)
         self.assertNotEqual(option1, option2)
 
+    def test_same_hash_if_equal(self):
+        option1 = TupleOption(length=2)
+        option2 = TupleOption(length=2)
+
+        self.assertEqual(option1, option2)
+        self.assertEqual(hash(option1), hash(option2))
+
 
 class TestDictOption(unittest.TestCase):
     cls = DictOption
@@ -1108,6 +1145,13 @@
         self.assertEqual(option1, option4)
         self.assertNotEqual(option1, option5)
 
+    def test_same_hash_if_equal(self):
+        option1 = DictOption()
+        option2 = DictOption(item=StringOption())
+
+        self.assertEqual(option1, option2)
+        self.assertEqual(hash(option1), hash(option2))
+
     def test_to_string_when_json(self):
         option = DictOption()
         result = option.to_string({'foo': '1'})
@@ -1238,6 +1282,13 @@
         self.assertNotEqual(section1, section3)
         self.assertNotEqual(section1, section4)
 
+    def test_same_hash_if_equal(self):
+        section1 = self.cls()
+        section2 = self.cls()
+
+        self.assertEqual(section1, section2)
+        self.assertEqual(hash(section1), hash(section2))
+
     def test_repr(self):
         """Test Section repr."""
         section1 = self.cls()