[PATCH 1/2] A function for resolving date format and order

Vratislav Podzimek vpodzime at redhat.com
Thu Oct 9 09:41:56 UTC 2014


On Wed, 2014-10-08 at 08:31 -0400, Anne Mulhern wrote:
> 
> 
> 
> ----- Original Message -----
> > From: "Vratislav Podzimek" <vpodzime at redhat.com>
> > To: "anaconda patch review" <anaconda-patches at 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 at redhat.com>
> > > > To: anaconda-patches at 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 at 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.
Yeah, I know and use any().

> 
> [mulhern at 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
Old-style classes wouldn't work, but whatever, I've changed 'any' to
'object'.

> 
> > > 
> > > > +    :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.
Working on keyboard and language/locale configuration taught me that
these functions are always buggy. I understand your point that the
current ValueError is only raise when there is an inconsistency in the
code. But I don't think we have to complicate things that much as you
suggest. What about the v3 patch I sent a minute ago?


> > > 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.
Still I don't get your point here. If the 'locale' utility is missing,
an error will be raise from execReadlines. If setlocale() doesn't work,
the test should PASS because the function it tests is supposed to work
for locales that work with setlocale(). The only case that could cause
troubles and wrong results of this test is if the 'locale' utility
misbehaved and returned something else than locales on its stdout. Do
you think I should add a test for that? Cause it is again something else
than this test tries to cover so it should be a separate test (not owned
and run by us, actually).

-- 
Vratislav Podzimek

Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic



More information about the anaconda-patches mailing list