[master/f21] Fix Size arithmatic operations (#1158792)

Brian C. Lane bcl at redhat.com
Tue Nov 4 17:20:18 UTC 2014


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)


More information about the anaconda-patches mailing list