Could you change the commit message to:
"Enlarge the number of arithmetic operations that evaluate to a Size object."
The meaning of "Fix" is something like "to make correct" or "to
make permanent"
and that seems wrong for this context.
This patch really,
*) leaves some operations that still behave incorrectly, like __div__ when other is Size
*) modifies some operations so that they continue to behave incorrectly, but differently.
*) modifies some operations so that they do behave correctly in same contexts, as
<number> * Size should yield Size.
*) modifies some operations so that they behave more incorrectly than previously.
Some of these are even mathematically nonsensical, setting aside any question of units.
If we think about the definition of exponentiation as repeated multiplications,
as x^2 == x * x, x ^3 = x * x * x, the weirdness of 2 ** Size("1 KiB") seems
pretty obvious.
The weirdness of 2 being multiplied by itself 1024 times evaluating to a Size() is only
slightly greater.
Anyway, I don't object to contents of the patch, because it does fix the current bug,
and I feel that _actually_ fixing these operations will take some effort and probably
persuasion.
But the commit message seems to me to be wrong, both in its
title and in its remarks about some controversy over Decimal / Size
(implying that there was nothing else in these op definitions that
anybody could even think of considering to be incorrect).
----- Original Message -----
From: "Brian C. Lane" <bcl(a)redhat.com>
To: anaconda-patches(a)lists.fedorahosted.org
Sent: Monday, November 3, 2014 3:17:40 PM
Subject: [master/f21] Fix Size arithmatic operations (#1158792)
When Size is not on the left it needs to call the correct Decimal method
and make sure it returns a Size object. This also adds a test to make
sure the results of add, subtract, multiply, divide, modulo and power
are all Size objects.
There is some debate about what a Decimal / Size should return. I think
Size makes sense because it will act like Decimal for everything except
display purposes.
---
blivet/size.py | 19 +++++++++++++++----
tests/size_test.py | 28 ++++++++++++++++++++++++++++
2 files changed, 43 insertions(+), 4 deletions(-)
diff --git a/blivet/size.py b/blivet/size.py
index 05a38b4..8220e21 100644
--- a/blivet/size.py
+++ b/blivet/size.py
@@ -235,25 +235,36 @@ class Size(Decimal):
def __add__(self, other, context=None):
return Size(Decimal.__add__(self, other, context=context))
-
- # needed to make sum() work with Size arguments
- def __radd__(self, other, context=None):
- return Size(Decimal.__radd__(self, other, context=context))
+ __radd__ = __add__
def __sub__(self, other, context=None):
# subtraction is implemented using __add__ and negation, so we'll
# be getting passed a Size
return Decimal.__sub__(self, other, context=context)
+ def __rsub__(self, other, context=None):
+ return Size(Decimal.__rsub__(self, other, context=context))
+
def __mul__(self, other, context=None):
return Size(Decimal.__mul__(self, other, context=context))
+ __rmul__ = __mul__
def __div__(self, other, context=None):
return Size(Decimal.__div__(self, other, context=context))
+ __rdiv__ = __div__
def __mod__(self, other, context=None):
return Size(Decimal.__mod__(self, other, context=context))
+ def __rmod__(self, other, context=None):
+ return Size(Decimal.__rmod__(self, other, context=context))
+
+ def __pow__(self, other, context=None):
+ return Size(Decimal.__pow__(self, other, context=context))
+
+ def __rpow__(self, other, context=None):
+ return Size(Decimal.__rpow__(self, other, context=context))
+
def convertTo(self, spec="b"):
""" Return the size in the units indicated by the specifier.
The
specifier can be prefixes from the _decimalPrefixes and
diff --git a/tests/size_test.py b/tests/size_test.py
index 9841b94..c6ed011 100644
--- a/tests/size_test.py
+++ b/tests/size_test.py
@@ -163,5 +163,33 @@ class SizeTestCase(unittest.TestCase):
os.environ['LANG'] = saved_lang
locale.setlocale(locale.LC_ALL, '')
+ def testArithmetic(self):
+ s = Size("2GiB")
+
+ # Make sure arithmatic operations with Size always result in Size
+ self.assertIsInstance(s+s, Size)
+ self.assertIsInstance(s-s, Size)
+ self.assertIsInstance(s*s, Size)
+ self.assertIsInstance(s/s, Size)
+ self.assertIsInstance(s**Size(2), Size)
+ self.assertIsInstance(s % Size(7), Size)
+
+
+ # Make sure operations with non-Size on the right result in Size
+ self.assertIsInstance(s+2, Size)
+ self.assertIsInstance(s-2, Size)
+ self.assertIsInstance(s*2, Size)
+ self.assertIsInstance(s/2, Size)
+ self.assertIsInstance(s**2, Size)
+ self.assertIsInstance(s % 127, Size)
+
+ # Make sure operations with non-Size on the left result in Size
+ self.assertIsInstance(2+s, Size)
+ self.assertIsInstance(2-s, Size)
+ self.assertIsInstance(2*s, Size)
+ self.assertIsInstance(2/s, Size)
+ self.assertIsInstance(2**Size(2), Size)
+ self.assertIsInstance(1024 % Size(127), Size)
+
if __name__ == "__main__":
unittest.main()
--
1.9.3
_______________________________________________
anaconda-patches mailing list
anaconda-patches(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches