----- Original Message -----
From: "Vratislav Podzimek" <vpodzime(a)redhat.com>
To: "anaconda patch review" <anaconda-patches(a)lists.fedorahosted.org>
Sent: Wednesday, October 8, 2014 3:19:50 AM
Subject: Re: [PATCH 1/2] A function for resolving date format and order
On Tue, 2014-10-07 at 17:29 -0400, Anne Mulhern wrote:
>
>
>
> ----- Original Message -----
> > From: "Vratislav Podzimek" <vpodzime(a)redhat.com>
> > To: anaconda-patches(a)lists.fedorahosted.org
> > Sent: Tuesday, October 7, 2014 12:32:36 PM
> > Subject: [PATCH 1/2] A function for resolving date format and order
> >
> > It's not that easy to get right order of date fields (year, month, day)
> > and
> > their formats so we should better have it in a standalone function with
> > tests.
> >
> > Related: rhbz#1044233
> > Signed-off-by: Vratislav Podzimek <vpodzime(a)redhat.com>
> > ---
> > pyanaconda/localization.py | 64
> > +++++++++++++++++++++++++++++
> > tests/pyanaconda_tests/localization_test.py | 21 ++++++++++
> > 2 files changed, 85 insertions(+)
> >
> > diff --git a/pyanaconda/localization.py b/pyanaconda/localization.py
> > index c95fa1d..efcb66f 100644
> > --- a/pyanaconda/localization.py
> > +++ b/pyanaconda/localization.py
> > @@ -26,6 +26,7 @@ import re
> > import langtable
> > import locale as locale_mod
> > import glob
> > +from collections import namedtuple
> >
> > from pyanaconda import constants
> > from pyanaconda.iutil import upcase_first_letter
> > @@ -469,3 +470,66 @@ def load_firmware_language(lang):
> >
> > log.debug("Using UEFI PlatformLang '%s' ('%s') as our
language.", d,
> > locales[0])
> > setup_locale(locales[0], lang)
> > +
> > +_DateFieldSpec = namedtuple("DateFieldSpec", ["format",
"suffix"])
> > +
> > +def resolve_date_format(year, month, day):
> > + """
> > + Puts the year, month and day objects in the right order according to
> > the
> > + currently set locale and provides format specification for each of
> > the
> > + fields.
> > +
> > + :param year: any object or value representing year
> > + :type year: any
> > + :param month: any object or value representing month
> > + :type month: any
> > + :param day: any object or value representing day
> > + :type day: any
>
> Uses of 'any' for type should be changed to 'object'.
Does 'object' involve ints, floats, etc.?
Yes. And any() is a function on an iterable.
[mulhern@localhost ~]$ python
Python 2.7.5 (default, Jun 19 2014, 12:16:30)
[GCC 4.8.2 20131212 (Red Hat 4.8.2-7)] on linux2
Type "help", "copyright", "credits" or "license"
for more information.
>> issubclass(int, object)
True
>
> > + :returns: a pair where the first field contains a tuple with the
> > year,
> > month
> > + and day objects/values put in the right order and where
> > the
> > second
> > + field contains a tuple with three :class:`_DateFieldSpec`
> > objects
> > + specifying formats respectively to the first (year, month,
> > day)
> > + field
> > + :rtype: tuple
> > + :raise ValueError: in case currently set locale has unsupported date
> > format
> > + (likely a bug in this function)
>
> "this" sounds like it is referring to the function being defined, i.e.,
> this function.
I'm sorry, but I don't see the issue here. "this" is supposed to refer
to this function.
> Seems weird to say this function will raise a ValueError if it has a bug.
That would be weird, but that's not what the documentation string says.
It says that a ValueError is raised in case currently set locale has
unsupported date format and it admits that such behaviour is likely a
bug in this function (because it should support such locale/date
format).
>
> > +
> > + """
> > +
> > + # see date (1), 'O' (not '0') is a mystery, 'E' is
Buddhist
> > calendar,
> > '(.*)'
> > + # is an arbitrary suffix
> > + field_spec_re = re.compile(r'([-_0OE^#]*)([yYmbBde])(.*)')
> > +
> > + order = []
> > + formats = []
> > + fmt_str = locale_mod.nl_langinfo(locale_mod.D_FMT)
> > +
> > + # see date (1)
> > + fmt_str = fmt_str.replace("%F", "%Y-%m-%d")
>
> You might want to point out here that while the Python
> documentation does not document '%F' as a valid format
> string it definitely exists and is in format string returned
> by nl_langinfo for some calls.
I'd say that's implied by the fact it is there.
>
> > + # e.g. "%d.%m.%Y" -> ['d.', 'm.',
'Y']
> > + fields = fmt_str.split("%")[1:]
> > +
>
> matches = (field_spec_re.match(f) for f in fields)
> fmts = [m.groups() for m in matches if m is not None]
>
> terms = []
> for _prefix, item, _suffix in fmts:
> if ...:
> terms.append(...)
> ...
>
> formats = [_DateFieldSpec("%" + prefix + item, suffix.strip()) for
> prefix, item, suffix in fmts]
> return (tuple(terms), tuple(formats))
>
> I'ld rather do the above than the below, but, in any case,
I must say I find the for-cycle below better readable and easier to
understand.
> I think terms, or ordered_terms, or sorted_terms, would be better than
> order.
Agreed, fixing locally.
Coming back to this, I realize that the problem is a deeper one.
This function raises a ValueError exactly when there is a bug,
i.e., when the if condition below is inconsistent with the the field_spec_re
by expecting fewer matching codes than the field_spec_re expects.
It doesn't raise a ValueError if the if condition expects more codes,
that would be silent. And it doesn't raise a ValueError if it encounters
some kind of unexpectedly formatted string, it would be silent in that case
as well.
If it were me, I would eliminate the possibility of inconsistency and
I would use the idea from your test to check if something got missed, like this:
fmt_str = locale_mod.nl_langinfo(locale_mod.D_FMT)
fmt_str = fmt_str.replace("%F", "%Y-%m-%d")
# Comment the regular expressions to indicate what they stand for, here
... # might be able to set up a sub-expression string here
year_spec_re = ...
month_spec_re = ...
day_spec_re = ...
regexes = [year_spec_re, month_spec_re, day_spec_re]
terms = [year, month, day]
ordered_terms = []
formats = []
# The fields in the format string, may include more than year, month, day fields
fields = fmt_str.split("%")[1:]
for field in fields:
for i, regex in enumerate(regexes):
match = regex.match(field)
if match:
ordered_terms.append(terms[i])
prefix, item, suffix = match.groups()
formats.append(_DateFieldSpec("%" + prefix + item,
suffix.strip()))
break
# fields should have contained exactly one entry corresponding to each of year, month,
day
if len(set(ordered_terms)) != 3:
raise ValueError(...)
return (tuple(ordered_terms), tuple(formats))
That way, you have no danger of failing due to inconsistency within your code,
and you do catch an unexpected format string if one occurs.
In method header I would say that a ValueError is raised in case an unexpected format
string
is encountered rather than if there is a bug in the function. If some new format
string is introduced then it is not a bug in the function...it's a new situation.
And presumably the function is not written with the expectation that it is buggy.
>
>
> > + for field in fields:
> > + match = field_spec_re.match(field)
> > + if not match:
> > + # ignore fields we are not interested in (like %A for
> > weekday
> > name, etc.)
> > + continue
> > +
> > + prefix, item, suffix = match.groups()
> > + if item in ("d", "e"):
> > + # "e" is the same as "_d"
> > + order.append(day)
> > + elif item in ("Y", "y"):
> > + # 4-digit year, 2-digit year
> > + order.append(year)
> > + elif item in ("m", "b", "B"):
> > + # month number, short month name, long month name
> > + order.append(month)
> > + else:
> > + raise ValueError("Unsupported date format's '%s'
field '%s'"
> > %
> > (fmt_str, field))
> > +
> > + # "%" + prefix + item gives a format for date/time
formatting
> > functions
> > + formats.append(_DateFieldSpec("%" + prefix + item,
> > suffix.strip()))
> > +
> > + return (tuple(order), tuple(formats))
> > diff --git a/tests/pyanaconda_tests/localization_test.py
> > b/tests/pyanaconda_tests/localization_test.py
> > index 00ff31d..5bf6cba 100644
> > --- a/tests/pyanaconda_tests/localization_test.py
> > +++ b/tests/pyanaconda_tests/localization_test.py
> > @@ -19,6 +19,8 @@
> > #
> >
> > from pyanaconda import localization
> > +from pyanaconda.iutil import execReadlines
> > +import locale as locale_mod
> > import unittest
> >
> > class ParsingTests(unittest.TestCase):
> > @@ -139,3 +141,22 @@ class
> > LangcodeLocaleMatchingTests(unittest.TestCase):
> > # no matches
> >
self.assertIsNone(localization.find_best_locale_match("pt_BR",
> > ["en_BR", "en"]))
> >
self.assertIsNone(localization.find_best_locale_match("cs_CZ.UTF-8",
> > ["en", "en.UTF-8"]))
> > +
> > + def resolve_date_format_test(self):
> > + """All locales' date formats should be properly
resolved."""
> > +
> > + locales = (line.strip() for line in execReadlines("locale",
> > ["-a"]))
> > + for locale in locales:
> > + try:
> > + locale_mod.setlocale(locale_mod.LC_ALL, locale)
> > + except locale_mod.Error:
> > + # cannot set locale (a bug in the locale module?)
> > + continue
> > +
> > + (order, formats) = localization.resolve_date_format(1, 2, 3)
> > + if len(order) != 3:
> > + self.fail("Not all date fields available for %s" %
> > locale)
> > + if len(formats) != 3:
> > + self.fail("Not all date fields' formats available
for
> > %s" %
> > locale)
> > + for i in (1, 2, 3):
> > + self.assertIn(i, order)
>
> You might want to count the number of times you hit the continue block and
> if
> it's an appreciable proportion, or all, of the number of locales, raise an
> exception so that the test has an error.
That'd be against the principles of unit testing. I'm not testing the
setlocale() function, I'm testing my function that is supposed to work
for locales that work with setlocale(). Such counting and test should be
a part Python's tests not our tests. And even if we wanted to test
setlocale() it should be a separate test.
I'm not interested in testing setlocale(), just in distinguishing between
none of the tests running and all of the tests passing. It has happened before.
I'ld want to see an ERROR in that case, not a FAIL, 'cause that's how
I'ld
interpret the situation.
--
Vratislav Podzimek
Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic
_______________________________________________
anaconda-patches mailing list
anaconda-patches(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
- mulhern