[blivet:master 1/3] Rewrite of Size.humanReadable() method

mulhern amulhern at redhat.com
Tue Sep 30 12:27:28 UTC 2014


The observable changes are:
*) Extremely large values will be displayed using the largest available units,
rather than as bytes.
*) The places parameter is eliminated entirely.
*) A strip parameter is added, with default True. If strip is True, and the
value is a fractional number, trailing zeros are stripped up to the decimal
point. strip is True by default to preserve current behavior.
*) A min_value parameter is added. The default is set to 10, to match the
current behavior of the method.

Remark:

Sizes are now stored in arbitrary precision Decimal values. The base unit
of computation is bytes, which is something that can be measured exactly.
Therefore, the quantities that are measured are precisely known, and,
barring mistakes, all computations should yield precise values.
Intuitions from other branches of engineering, where there is an
imprecision in measurement, or from calculating with float types,
which have limited precision, do not apply here. We do not need to display
all the precision that is available, but that shouldn't confuse us into
believing that it is not available.

Rationale for externally visible changes:

* Removal of places param.
Anaconda never sets places at all, and it always specifies max_places with
a keyword. Openlmi does not appear to use humanReadable at all.
The use of places was barely tested in the existing unittests and its
purpose is obscure. Removing it simplifies the code a good deal but does
not itself change any existing behavior (other than in test code) since
its default was None.

* Addition of strip parameter.
Some users will surely have an engineering background, like myself, and will
expect the significant digits that they request. strip is set to True,
however, so that the default behavior is removing 0s, since that is what
anaconda expects.

Internal changes and rationale:

* Obtain fractional digits by using Decimal.quantize(), not round,
which operates by conversion to float.

Since Decimals are arbitrary precision, every digit is significant.
Rounding should occur on the Decimal value, not by conversion to float.

* Returning prefix unconditionally.

abbr is set exactly when prefix is set, and they are only set from an
array which always contains both an abbr and a prefix. So falling back
on using the prefix when there is no abbreviation makes no sense.
On the other hand, it is not wrong to keep the prefix value in the array of
prefixes, it might make sense to add a use_prefix parameter to humanReadable,
for example.

* Search only for binary prefixes, not decimal ones.
Previously, the code searched among decimal prefixes and abbreviations if it
could not find a binary prefix that worked. As it happens, our binary prefixes
express a larger range than our decimal prefixes, so no decimal prefix
should ever have been chosen. Also, humanReadable() should not return
either binary or decimal prefix, depending on the value of the number
represented. Two reasons to ditch this behavior and only search for binary
prefixes.
Since we are dealing exclusively with binary prefixes we can use
1024 as our limit when searching for the correct prefix, not 1000, which
evidently was supposed to work for both kinds of prefixes.

* Add an _emptyPrefix constant and use it.
Previously, very small numbers that should be represented as bytes, w/out
prefix were handled especially. There was no need for this, so that's
eliminated.

* Other more complicated and hard to explain but not effective behavior
also eliminated.
The net result is the removal of 23 lines of code, leaving just 17,
and the addition of some comments making the method overall a good deal
more readable and parameterizable.

New method passes all existing tests without any alteration, except for
a couple tests that used places and now use max_places.

Signed-off-by: mulhern <amulhern at redhat.com>
---
 blivet/size.py     | 106 ++++++++++++++++++++++++++++-------------------------
 tests/size_test.py |  60 +++++++++++++++++++++++++++++-
 2 files changed, 114 insertions(+), 52 deletions(-)

diff --git a/blivet/size.py b/blivet/size.py
index e95de6a..8cabe24 100644
--- a/blivet/size.py
+++ b/blivet/size.py
@@ -19,6 +19,7 @@
 #
 # Red Hat Author(s): David Cantrell <dcantrell at redhat.com>
 
+import itertools
 import re
 import string
 import locale
@@ -30,7 +31,7 @@ from decimal import ROUND_DOWN
 import six
 
 from .errors import SizePlacesError
-from .i18n import _, P_, N_
+from .i18n import _, N_
 
 
 # Container for size unit prefix information
@@ -59,6 +60,9 @@ _binaryPrefixes = [_Prefix(1024, N_(b"kibi"), N_(b"Ki")),
                    _Prefix(1024**7, N_(b"zebi"), N_(b"Zi")),
                    _Prefix(1024**8, N_(b"yobi"), N_(b"Yi"))]
 
+# Empty prefix works both for decimal and binary
+_emptyPrefix = _Prefix(1, "".decode("utf-8"), "".decode("utf-8"))
+
 _bytes = [N_(b'B'), N_(b'b'), N_(b'byte'), N_(b'bytes')]
 _prefixes = _binaryPrefixes + _decimalPrefixes
 
@@ -273,70 +277,72 @@ class Size(Decimal):
 
         return None
 
-    def humanReadable(self, places=None, max_places=2):
+    def humanReadable(self, max_places=2, strip=True, min_value=10):
         """ Return a string representation of this size with appropriate
-            size specifier and in the specified number of decimal places
-            (i.e. the maximal precision is only achieved by setting both places
-            and max_places to None).
+            size specifier and in the specified number of decimal places.
+            Values are always represented using binary not decimal units.
+            For example, if the number of bytes represented by this size
+            is 65531, expect the representation to be something like
+            64.00 KiB, not 65.53 KB.
+
+            :param max_places: number of decimal places to use, default is 2
+            :type max_places: an integer type or NoneType
+            :param bool strip: True if trailing zeros are to be stripped.
+            :param min_value: Minimum value on lhs of decimal point.
+            :type min_value: An integer type or NoneType
+            :returns: a representation of the size
+            :rtype: str
+
+            If max_places is set to None, all non-zero digits will be shown.
+            Otherwise, max_places digits will be shown.
+
+            If strip is True and there is a fractional quantity, trailing
+            zeros are removed up to the decimal point.
+
+            min_value sets the smallest value allowed on the lhs of the
+            decimal. If min_value is 10, then single digits on the lhs of
+            the decimal will be avoided if possible. In that case,
+            9216 KiB is preferred to 9 MiB. However, 1 B has no alternative.
+            If min_value is 1, however, 9 MiB is preferred.
+
+            humanReadable() is a function that evaluates to a number which
+            represents a range of values. For a constant choice of max_places,
+            all ranges are of equal size, and are bisected by the result. So,
+            if n.humanReadable() == x U and b is the number of bytes in 1 U,
+            and e = 1/2 * 1/(10^max_places) * b,
+            then x - e < n < x + e.
         """
-        if places is not None and places < 0:
-            raise SizePlacesError("places= must be >=0 or None")
-
-        if max_places is not None and max_places < 0:
-            raise SizePlacesError("max_places= must be >=0 or None")
+        if max_places is not None and (max_places < 0 or not isinstance(max_places, six.integer_types)):
+            raise SizePlacesError("max_places must be None or an non-negative integer value")
 
-        in_bytes = int(Decimal(self))
-        if abs(in_bytes) < 1000:
-            return "%d %s" % (in_bytes, _("B"))
+        if min_value < 0 or not isinstance(min_value, six.integer_types):
+            raise ValueError("min_value must be a positive integer value.")
 
-        prev_prefix = None
-        for prefix_item in _xlated_prefixes():
-            factor, prefix, abbr = prefix_item
+        # Find the smallest prefix which will allow a number less than
+        # 1024 * min_value to the left of the decimal point. If the number is
+        # so large that no prefix will satisfy this requirement use the
+        # largest prefix.
+        for factor, _prefix, abbr in itertools.chain([_emptyPrefix], _xlated_binary_prefixes()):
             newcheck = super(Size, self).__div__(Decimal(factor))
 
-            if abs(newcheck) < 1000:
+            if abs(newcheck) < 1024 * min_value:
                 # nice value, use this factor, prefix and abbr
                 break
-            prev_prefix = prefix_item
-        else:
-            # no nice value found, just return size in bytes
-            return "%s %s" % (in_bytes, _("B"))
-
-        if abs(newcheck) < 10:
-            if prev_prefix is not None:
-                factor, prefix, abbr = prev_prefix # pylint: disable=unpacking-non-sequence
-                newcheck = super(Size, self).__div__(Decimal(factor))
-            else:
-                # less than 10 KiB
-                return "%s %s" % (in_bytes, _("B"))
-
-        retval = newcheck
-        if places is not None:
-            retval = round(newcheck, places)
 
         if max_places is not None:
-            if places is not None:
-                limit = min((places, max_places))
-            else:
-                limit = max_places
-            retval = round(newcheck, limit)
+            newcheck = newcheck.quantize(Decimal(10) ** -max_places)
+
+        retval_str = str(newcheck)
 
-        if retval == int(retval):
-            # integer value, no point in showing ".0" at the end
-            retval = int(retval)
+        if '.' in retval_str and strip:
+            retval_str = retval_str.rstrip("0").rstrip(".")
 
-        # Format the value with '.' as the decimal separator
         # If necessary, substitute with a localized separator before returning
-        retval_str = str(retval)
         radix = locale.nl_langinfo(locale.RADIXCHAR)
         if radix != '.':
             retval_str = retval_str.replace('.', radix)
 
-        # abbr and prefix are unicode objects so that lower/upper work correctly
-        # Convert them to str before concatenating so that the return type is
-        # str.
+        # Convert unicode objects to str before concatenating so that the
+        # resulting expression is a str.
         # pylint: disable=undefined-loop-variable
-        if abbr:
-            return retval_str + " " + abbr.encode("utf-8") + _("B")
-        else:
-            return retval_str + " " + prefix.encode("utf-8") + P_("byte", "bytes", newcheck)
+        return retval_str + " " + abbr.encode("utf-8") + _("B")
diff --git a/tests/size_test.py b/tests/size_test.py
index 9841b94..294e5d9 100644
--- a/tests/size_test.py
+++ b/tests/size_test.py
@@ -38,9 +38,9 @@ class SizeTestCase(unittest.TestCase):
 
         s = Size(500)
         with self.assertRaises(SizePlacesError):
-            s.humanReadable(places=-1)
+            s.humanReadable(max_places=-1)
 
-        self.assertEqual(s.humanReadable(places=0), "500 B")
+        self.assertEqual(s.humanReadable(max_places=0), "500 B")
 
     def _prefixTestHelper(self, numbytes, factor, prefix, abbr):
         c = numbytes * factor
@@ -104,6 +104,62 @@ class SizeTestCase(unittest.TestCase):
         s = Size("12.6998 TiB")
         self.assertEquals(s.humanReadable(max_places=2), "12.7 TiB")
 
+        # byte values close to multiples of 2 are shown without trailing zeros
+        s = Size(0xfff)
+        self.assertEquals(s.humanReadable(max_places=2), "4095 B")
+        s = Size(8193)
+        self.assertEquals(s.humanReadable(max_places=2), "8193 B")
+
+        # a fractional quantity is shown if the value deviates
+        # from the whole number of units by more than 1%
+        s = Size(16384 - (1024/100 + 1))
+        self.assertEquals(s.humanReadable(max_places=2), "15.99 KiB")
+
+        # if max_places is set to None, all digits are displayed
+        s = Size(0xfffffffffffff)
+        self.assertEquals(s.humanReadable(max_places=None), "4095.999999999999090505298227 TiB")
+        s = Size(0x10000)
+        self.assertEquals(s.humanReadable(max_places=None), "64 KiB")
+        s = Size(0x10001)
+        self.assertEquals(s.humanReadable(max_places=None), "64.0009765625 KiB")
+
+        # test a very large quantity with no associated abbreviation or prefix
+        s = Size(1024**9)
+        self.assertEquals(s.humanReadable(max_places=2), "1024 YiB")
+        s = Size(1024**9 - 1)
+        self.assertEquals(s.humanReadable(max_places=2), "1024 YiB")
+        s = Size(1024**9 + 1)
+        self.assertEquals(s.humanReadable(max_places=2, strip=False), "1024.00 YiB")
+        s = Size(1024**10)
+        self.assertEquals(s.humanReadable(max_places=2), "1048576 YiB")
+
+    def testHumanReadableFractionalQuantities(self):
+        s = Size(0xfffffffffffff)
+        self.assertEquals(s.humanReadable(max_places=2), "4096 TiB")
+        s = Size(0xfffff)
+        self.assertEquals(s.humanReadable(max_places=2, strip=False), "1024.00 KiB")
+        s = Size(0xffff)
+        # value is not exactly 64 KiB, but w/ 2 places, value is 64.00 KiB
+        # so the trailing 0s are stripped.
+        self.assertEquals(s.humanReadable(max_places=2), "64 KiB")
+        # since all significant digits are shown, there are no trailing 0s.
+        self.assertEquals(s.humanReadable(max_places=None), "63.9990234375 KiB")
+
+        # deviation is less than 1/2 of 1% of 1024
+        s = Size(16384 - (1024/100/2))
+        self.assertEquals(s.humanReadable(max_places=2), "16 KiB")
+        # deviation is greater than 1/2 of 1% of 1024
+        s = Size(16384 - ((1024/100/2) + 1))
+        self.assertEquals(s.humanReadable(max_places=2), "15.99 KiB")
+
+        s = Size(0x10000000000000)
+        self.assertEquals(s.humanReadable(max_places=2), "4096 TiB")
+
+    def testMinValue(self):
+        s = Size("9 MiB")
+        self.assertEquals(s.humanReadable(min_value=1), "9 MiB")
+        self.assertEquals(s.humanReadable(), "9216 KiB")
+
     def testConvertToPrecision(self):
         s = Size(1835008)
         self.assertEquals(s.convertTo(spec="b"), 1835008)
-- 
1.9.3



More information about the anaconda-patches mailing list