Bug 1379298 - Relax __eq__ for empty OptionValue; r?rillian draft
authorGregory Szorc <gps@mozilla.com>
Mon, 10 Jul 2017 11:21:37 -0700
changeset 606238 be5a40e9efa102ea692c1d242a82d5e68ad11e48
parent 606124 91c943f7373722ad4e122d98a2ddd6c79708b732
child 636719 dde6571634cbf9856bc126942de51eb4f34acdf6
push id67651
push userbmo:gps@mozilla.com
push dateMon, 10 Jul 2017 19:01:51 +0000
reviewersrillian
bugs1379298, 1375231
milestone56.0a1
Bug 1379298 - Relax __eq__ for empty OptionValue; r?rillian The rigid type comparison added in 51a92a22d6d1 (bug 1375231) was too rigid. This broke at least one consumer that was comparing an empty PositiveOptionValue/NegativeOptionValue against a string. Since comparisons against empty OptionValue are convenient and don't violate the spirit of the type checking previously added, this commit relaxes the type equivalence check in cases where the OptionValue is empty. MozReview-Commit-ID: UllTrzCjj
python/mozbuild/mozbuild/configure/options.py
python/mozbuild/mozbuild/test/configure/test_options.py
--- a/python/mozbuild/mozbuild/configure/options.py
+++ b/python/mozbuild/mozbuild/configure/options.py
@@ -51,21 +51,24 @@ class OptionValue(tuple):
             return option
         elif self and not len(self):
             return '%s=1' % option
         return '%s=%s' % (option, ','.join(self))
 
     def __eq__(self, other):
         # This is to catch naive comparisons against strings and other
         # types in moz.configure files, as it is really easy to write
-        # value == 'foo'.
-        if not isinstance(other, tuple):
-            raise TypeError('cannot compare a %s against an %s; OptionValue '
-                            'instances are tuples - did you mean to compare '
-                            'against member elements using [x]?' % (
+        # value == 'foo'. We only raise a TypeError for instances that
+        # have content, because value-less instances (like PositiveOptionValue
+        # and NegativeOptionValue) are common and it is trivial to
+        # compare these.
+        if not isinstance(other, tuple) and len(self):
+            raise TypeError('cannot compare a populated %s against an %s; '
+                            'OptionValue instances are tuples - did you mean to '
+                            'compare against member elements using [x]?' % (
                                 type(other).__name__, type(self).__name__))
 
         # Allow explicit tuples to be compared.
         if type(other) == tuple:
             return tuple.__eq__(self, other)
         # Else we're likely an OptionValue class.
         elif type(other) != type(self):
             return False
--- a/python/mozbuild/mozbuild/test/configure/test_options.py
+++ b/python/mozbuild/mozbuild/test/configure/test_options.py
@@ -292,16 +292,30 @@ class TestOption(unittest.TestCase):
         # Different OptionValue types are never equal.
         self.assertNotEqual(val, OptionValue(('foo',)))
 
         # For usability reasons, we raise TypeError when attempting to compare
         # against a non-tuple.
         with self.assertRaisesRegexp(TypeError, 'cannot compare a'):
             val == 'foo'
 
+        # But we allow empty option values to compare otherwise we can't
+        # easily compare value-less types like PositiveOptionValue and
+        # NegativeOptionValue.
+        empty_positive = PositiveOptionValue()
+        empty_negative = NegativeOptionValue()
+        self.assertEqual(empty_positive, ())
+        self.assertEqual(empty_positive, PositiveOptionValue())
+        self.assertEqual(empty_negative, ())
+        self.assertEqual(empty_negative, NegativeOptionValue())
+        self.assertNotEqual(empty_positive, 'foo')
+        self.assertNotEqual(empty_positive, ('foo',))
+        self.assertNotEqual(empty_negative, 'foo')
+        self.assertNotEqual(empty_negative, ('foo',))
+
     def test_option_value_format(self):
         val = PositiveOptionValue()
         self.assertEquals('--with-value', val.format('--with-value'))
         self.assertEquals('--with-value', val.format('--without-value'))
         self.assertEquals('--enable-value', val.format('--enable-value'))
         self.assertEquals('--enable-value', val.format('--disable-value'))
         self.assertEquals('--value', val.format('--value'))
         self.assertEquals('VALUE=1', val.format('VALUE'))