Bug 1379298 - Relax __eq__ for empty OptionValue; r=nalexander,rillian
authorGregory Szorc <gps@mozilla.com>
Mon, 10 Jul 2017 11:21:37 -0700
changeset 368146 9b8398ced330159a68a772cc25e9c33609c2a44f
parent 368145 914fc90581c481dc80b97da80e22b42c69f1068d
child 368147 78f86956e19190ecbfc42b5a7766eab1838507e9
push id32158
push usercbook@mozilla.com
push dateTue, 11 Jul 2017 10:48:59 +0000
treeherdermozilla-central@5e2692f8a367 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnalexander, rillian
bugs1379298, 1375231
milestone56.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1379298 - Relax __eq__ for empty OptionValue; r=nalexander,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'))