[blivet 2/2] Accept both English and localized sizes in Size specs.

David Shea dshea at redhat.com
Tue Jan 21 14:36:27 UTC 2014


On 01/21/2014 02:31 AM, Vratislav Podzimek wrote:
> On Mon, 2014-01-20 at 17:26 -0500, David Shea wrote:
>> _parseSpec will accept both English and localized size specs, either
>> either a period or a localized radix character. Size.humanReadable will
> Double 'either'.

Oops, fixed.

>
>> always return the localized version of a Size.
>> ---
>>   blivet/size.py     | 86 +++++++++++++++++++++++++++++++++++++++---------------
>>   tests/size_test.py | 42 ++++++++++++++++++++++++++
>>   2 files changed, 105 insertions(+), 23 deletions(-)
>>
>> diff --git a/blivet/size.py b/blivet/size.py
>> index a0d572d..3969c15 100644
>> --- a/blivet/size.py
>> +++ b/blivet/size.py
>> @@ -20,6 +20,8 @@
>>   # Red Hat Author(s): David Cantrell <dcantrell at redhat.com>
>>   
>>   import re
>> +import string
>> +import locale
>>   
>>   from decimal import Decimal
>>   from decimal import InvalidOperation
>> @@ -54,17 +56,30 @@ _binaryPrefix = [(1024, N_("kibi"), N_("Ki")),
>>   _bytes = [N_('B'), N_('b'), N_('byte'), N_('bytes')]
>>   _prefixes = _binaryPrefix + _decimalPrefix
>>   
>> -def _makeSpecs(prefix, abbr):
>> +_ASCIIlower_table = string.maketrans(string.ascii_uppercase, string.ascii_lowercase)
>> +def _lowerASCII(s):
>> +    """Convert a string to lowercase using only ASCII character definitions."""
>> +    return string.translate(s, _ASCIIlower_table)
>> +
>> +def _makeSpecs(prefix, abbr, xlate):
>>       """ Internal method used to generate a list of specifiers. """
>>       specs = []
>>   
>>       if prefix:
>> -        specs.append(prefix.lower() + _("byte"))
>> -        specs.append(prefix.lower() + _("bytes"))
>> +        if xlate:
>> +            specs.append(prefix.lower() + _("byte").decode("utf-8"))
>> +            specs.append(prefix.lower() + _("bytes").decode("utf-8"))
>> +        else:
>> +            specs.append(_lowerASCII(prefix) + "byte")
>> +            specs.append(_lowerASCII(prefix) + "bytes")
>>   
>>       if abbr:
>> -        specs.append(abbr.lower() + _("b"))
>> -        specs.append(abbr.lower())
>> +        if xlate:
>> +            specs.append(abbr.lower() + _("b").decode("utf-8"))
>> +            specs.append(abbr.lower())
>> +        else:
>> +            specs.append(_lowerASCII(abbr) + "b")
>> +            specs.append(_lowerASCII(abbr))
>>   
>>       return specs
>>   
>> @@ -73,11 +88,15 @@ def _parseSpec(spec):
>>       if not spec:
>>           raise ValueError("invalid size specification", spec)
>>   
>> -    # This regex isn't ideal, since \w matches both letters and digits,
>> -    # but python doesn't provide a means to match only Unicode letters.
>> -    # Probably the worst that will come of it is that bad specs will fail
>> -    # more confusingly.
>> -    m = re.match(r'(-?\s*[0-9.]+)\s*(\w*)$', spec.decode("utf-8").strip(), flags=re.UNICODE)
>> +    # Replace the localized radix character with a .
>> +    radix = locale.nl_langinfo(locale.RADIXCHAR)
>> +    if radix != '.':
>> +        spec = spec.replace(radix, '.')
>> +
>> +    # Match the string using only digit/space/not-space, since the
>> +    # string might be non-English and contain non-letter characters
>> +    # that Python doesn't understand as parts of words.
>> +    m = re.match(r'(-?\s*[0-9.]+)\s*([^\s]*)$', spec.strip())
>>       if not m:
>>           raise ValueError("invalid size specification", spec)
>>   
>> @@ -86,14 +105,30 @@ def _parseSpec(spec):
>>       except InvalidOperation:
>>           raise ValueError("invalid size specification", spec)
>>   
>> -    specifier = m.groups()[1].lower()
>> -    bytes_xlated = [_(b) for b in _bytes]
>> -    if not specifier or specifier in bytes_xlated:
>> +    # Only attempt to parse as English if all characters are ASCII
>> +    try:
>> +        specifier = _lowerASCII(str(m.groups()[1].decode('ascii').lower()))
> Could you please try to split this into multiple lines? If an exception
> was raised in such line, it would be really hard to tell in which part.
> Also assigning results of independent steps into properly named
> variables will make the code clearer.

Ok.

>
>> +    except UnicodeDecodeError:
>> +        pass
>> +    else:
>> +        if specifier in _bytes or not specifier:
>> +            return size
>> +
>> +        for factor, prefix, abbr in _prefixes:
>> +            check = _makeSpecs(prefix, abbr, False)
>> +
>> +            if specifier in check:
>> +                return size * factor
>> +
>> +    # No English match found, try localized size specs
>> +    xlated_bytes = [_(b).decode("utf-8") for b in _bytes]
>> +    specifier = m.groups()[1].decode("utf-8").lower()
>> +    if specifier in xlated_bytes:
>>           return size
>>   
>> -    prefixes_xlated = [_(p) for p in _prefixes]
>> -    for factor, prefix, abbr in prefixes_xlated:
>> -        check = _makeSpecs(prefix, abbr)
>> +    xlated_prefixes = [(p[0], _(p[1]).decode("utf-8"), _(p[2]).decode("utf-8")) for p in _prefixes]
> A thing for another patch probably, but could _prefixes be a list of
> namedtuples with named fields? This lines looks like magic to me. Also
> having a function xlate_prefix and mapping it on _prefixes may be
> better, hiding the details of the implementation from the code that
> doesn't have to care about it.

Yeah, it'd be better off in a function at the least.

>
>> +    for factor, prefix, abbr in xlated_prefixes:
>> +        check  = _makeSpecs(prefix, abbr, True)
>>   
>>           if specifier in check:
>>               return size * factor
>> @@ -192,13 +227,11 @@ class Size(Decimal):
>>           """
>>           spec = spec.lower()
>>   
>> -        bytes_xlated = [_(b) for b in _bytes]
>> -        if spec in bytes_xlated:
>> +        if spec in _bytes:
>>               return self
>>   
>> -        prefixes_xlated = [_(p) for p in _prefixes]
>> -        for factor, prefix, abbr in prefixes_xlated:
>> -            check = _makeSpecs(prefix, abbr)
>> +        for factor, prefix, abbr in _prefixes:
>> +            check = _makeSpecs(prefix, abbr, False)
>>   
>>               if spec in check:
>>                   return Decimal(self / Decimal(factor))
>> @@ -221,7 +254,7 @@ class Size(Decimal):
>>           if abs(Decimal(check)) < 1000:
>>               return "%s %s" % (check, _("B"))
>>   
>> -        prefixes_xlated = [_(p) for p in _prefixes]
>> +        prefixes_xlated = [(p[0], _(p[1]), _(p[2])) for p in _prefixes]
>>           for factor, prefix, abbr in prefixes_xlated:
>>               newcheck = super(Size, self).__div__(Decimal(factor))
>>   
>> @@ -229,6 +262,8 @@ class Size(Decimal):
>>                   # nice value, use this factor, prefix and abbr
>>                   break
>>   
>> +        # Format the value with '.' as the decimal separator
>> +        # If necessary, substitute with a localized separator before returning
>>           if places is not None:
>>               newcheck_str = str(newcheck)
>>               retval = newcheck_str
>> @@ -238,13 +273,18 @@ class Size(Decimal):
>>           else:
>>               retval = self._trimEnd(str(newcheck))
>>   
>> +        radix = locale.nl_langinfo(locale.RADIXCHAR)
>> +
>>           if max_places is not None:
>>               (whole, point, fraction) = retval.partition(".")
>>               if point and len(fraction) > max_places:
>>                   if max_places == 0:
>>                       retval = whole
>>                   else:
>> -                    retval = "%s.%s" % (whole, fraction[:max_places])
>> +                    retval = "%s%s%s" % (whole, radix, fraction[:max_places])
>> +
>> +        if radix != '.':
>> +            retval = retval.replace('.', radix)
>>   
>>           if abbr:
>>               return retval + " " + abbr + _("B")
>> diff --git a/tests/size_test.py b/tests/size_test.py
>> index 30bc6eb..fc1df2b 100644
>> --- a/tests/size_test.py
>> +++ b/tests/size_test.py
>> @@ -87,6 +87,48 @@ class SizeTestCase(unittest.TestCase):
>>           self.assertEquals(Size(spec="%s KiB" % (1/1025.0,)), Size(bytes=0))
>>           self.assertEquals(Size(spec="%s KiB" % (1/1023.0,)), Size(bytes=1))
>>   
>> +    def testTranslated(self):
>> +        import locale
>> +        import os
>> +        from blivet.i18n import _
>> +        import gettext
>> +
>> +        saved_lang = os.environ.get('LANG', None)
>> +
>> +        # es_ES uses latin-characters but a comma as the radix separator
>> +        # kk_KZ uses non-latin characters and is case-sensitive
>> +        # te_IN uses a lot of non-letter modifier characters
>> +        test_langs = ["es_ES.UTF-8", "kk_KZ.UTF-8", "ml_IN.UTF-8"]
>> +
>> +        s = Size(spec="56.19 MiB")
>> +        for lang in test_langs:
>> +            os.environ['LANG'] = lang
>> +            locale.setlocale(locale.LC_ALL, '')
>> +
>> +            # Check English parsing
>> +            self.assertEquals(s, Size(spec="56.19 MiB"))
>> +
>> +            # Check native parsing
>> +            self.assertEquals(s, Size(spec="56.19 %s%s" % (_("Mi"), _("B"))))
>> +
>> +            # Check native parsing, all lowercase
>> +            self.assertEquals(s, Size(spec=("56.19 %s%s" % (_("Mi"), _("B"))).lower()))
>> +
>> +            # Check native parsing, all uppercase
>> +            self.assertEquals(s, Size(spec=("56.19 %s%s" % (_("Mi"), _("B"))).upper()))
>> +
>> +            # If the radix separator is not a period, repeat the tests with the
>> +            # native separator
>> +            radix = locale.nl_langinfo(locale.RADIXCHAR)
>> +            if radix != '.':
>> +                self.assertEquals(s, Size(spec="56%s19 MiB" % radix))
>> +                self.assertEquals(s, Size(spec="56%s19 %s%s" % (radix, _("Mi"), _("B"))))
>> +                self.assertEquals(s, Size(spec=("56%s19 %s%s" % (radix, _("Mi"), _("B"))).lower()))
>> +                self.assertEquals(s, Size(spec=("56%s19 %s%s" % (radix, _("Mi"), _("B"))).upper()))
>> +
>> +        os.environ['LANG'] = saved_lang
>> +        locale.setlocale(locale.LC_ALL, '')
> You should probably restore the $LC_ALL value here as well not just set it to an empty string.
>

Calling setlocale with an empty string sets the locale based on the 
environment, which is being restored.


More information about the anaconda-patches mailing list