On Tue, Nov 04, 2014 at 08:09:29AM -0500, Anne Mulhern wrote:
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.
My meaning was that it fixed incorrect operations, like 2 * Size
returning Decimal which was the root of the bug.
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.
Which ones?
*) 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.
Which ones?
Those 2 points above seem like grounds to reject the patch.
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.
The idea here was that when you're including a Size in your calculation
you are planning on presenting it to the user at some point, so results
should be Size.
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).
Not including a comprehensive list of possible objections does not imply
that there could not be anything else objectionable.
My goals here are:
* Fix the bug
* Don't introduce new ones
* Try to make Size act in an unsurprising manner.
* Add tests, since there were none focused on the actual operations
I also see that you're working on an extensive rewrite of Size, so maybe
I should restrict this patch to just fixing the bug (eg. __rmul__ =
__mul__) and add tests that reflect current behavior of the operations.
--
Brian C. Lane | Anaconda Team | IRC: bcl #anaconda | Port Orchard, WA (PST8PDT)