[ZCM] [ZC] 2191/21 Comment "DateTime doesn't parse all ISO8601-valid strings"
"Collector: Zope Bugs, Features, and Patches ..." <[email protected]> Fri, 23 Mar 2007 01:12:47 -0400
| Newsgroups | gmane.comp.web.zope.devel.collector-monitor |
|---|---|
| Message-ID | <[email protected]> |
Issue #2191 Update (Comment) "DateTime doesn't parse all ISO8601-valid strings"
Status Accepted, Zope/bug medium
To followup, visit:
http://www.zope.org/Collectors/Zope/2191
==============================================================
= Comment - Entry #21 by bacarter on Mar 23, 2007 1:12 am
> Again, the wikipedia page you cite doesn't support that; it says:
>
> If no time zone information is given with a time, the time zone is
> assumed to be in some conventional local timezone. While it may be
> safe to assume a local zone when used between two people in the same
> area, it is ambiguous when used in communication between multiple
> timezones. It is usually preferable to indicate a time zone using the
> standard’s notation.
Agreed Wikipedia isn't the standard, but it's a simple description of an (ahem) detailed specification.
> The "some conventional local timezone" makes the choice of zone an
> implmentation detail: in Zope's case, we chose UTC.
>
I disagree. It's fairly clear by the word 'local' that the timezone should be based on the localtime in my mind.
In fact, the spec you cite clearly defines 'local time' (2.1.16) as:
"locally applicable time of day such as standard time of day, or a *non-UTC* based time of day"
More importantly, as I pointed out before, this patch breaks backwards compatibility with previous versions of zope. In my mind, and in the pythonic way, that trumps specification correctness any day.
As further evidence, ZMySQLDA does a simple DateTime() cast on mysql date, timestamp, and datetime columns:
>From ZMySQLDA/db.py, line 144:
def DateTime_or_None(s):
try: return DateTime(s)
except: return None
Thus prior to zope 2.9.6, a Date column in mysql returned via ZMySQL are in localtime. As of 2.9.6 they're now all in GMT.
________________________________________
= Comment - Entry #20 by tseaver on Mar 18, 2007 9:56 am
> from here:
> http://en.wikipedia.org/wiki/ISO_8601#Combined_representations
> the whitespace is allowed to separate date and time
First, wikipedia is not authoritative against the spec, which I
cited below.
Second, it happens to be correct here, where it says:
The date and time representations may sometimes appear in
proximity, separated by a space or other characters, in which
case they occupy two separate fields in a data system, rather
than a single combined representation.
In fact, the only datetime format it shows it describes as
'<date>T<time>'.
> bacarter, eikenberry,
>
> I found out that the method __parse_iso8601 sets the TimeZone to GMT+0
> when a timezone is not given with the time. This is not allowed in
> ISO8601. In the case when a timezone is not given local timezone should
> be used.
Again, the wikipedia page you cite doesn't support that; it says:
If no time zone information is given with a time, the time zone is
assumed to be in some conventional local timezone. While it may be
safe to assume a local zone when used between two people in the same
area, it is ambiguous when used in communication between multiple
timezones. It is usually preferable to indicate a time zone using the
standard’s notation.
The "some conventional local timezone" makes the choice of zone an
implmentation detail: in Zope's case, we chose UTC.
________________________________________
= Comment - Entry #19 by mustapha on Mar 17, 2007 12:28 pm
Hi,
from here:
http://en.wikipedia.org/wiki/ISO_8601#Combined_representations
the whitespace is allowed to separate date and time
bacarter, eikenberry,
I found out that the method __parse_iso8601 sets the TimeZone to GMT+0 when a timezone is not given with the time. This is not allowed in ISO8601. In the case when a timezone is not given local timezone should be used.
I modified the line before the 'return' statement in __parse_iso8601 method from:
tz = 'GMT%+03d%02d' % (hour_off, min_off)
To:
tz = fields['hour_off'] and 'GMT%+03d%02d' % (hour_off, min_off) or None
That way the method _parse_args will set tz to the local timezone if no tz is given.
This modification makes the test in testDateTime.testISO8601 method fail at
self.assertEqual(ref0, isoDt)
with:
ref0 = DateTime('2002/5/2 8:00am GMT')
isoDt = DateTime('2002-05-02T08:00:00') #here will tz = local timezone
and that's normal: the assertEqual returns True only if your local timezone is GMT+0 but if you are leaving at GMT+1 like me the assertEqual will fail.
________________________________________
= Comment - Entry #18 by eikenberry on Feb 27, 2007 2:48 pm
The regex has a line that allows a whitespace where it shouldn't.
Around line 19 of the regex is this line.
(?:[T ] # one T or one whitespace
This should be (or some proximate of this).
(?:T? # one optional T
>From my reading of the spec this whitespace is not allowed. Below is the section of the document that discusses this. It mentions the 'T' being optionally omitted, but nothing about allowing a whitespace.
--- snippet from page 19 of the iso8601 spec ---
The character [T] shall be used as time designator to indicate the start of the representation of the time of day
component in these expressions. The hyphen [-] and the colon [:] shall be used, in accordance with 4.4.4, as
separators within the date and time of day expressions, respectively, when required.
NOTE By mutual agreement of the partners in information interchange, the character [T] may be omitted in
applications where there is no risk of confusing a date and time of day representation with others defined in this
International Standard.
________________________________________
= Comment - Entry #17 by tseaver on Feb 27, 2007 1:57 pm
Agreed, the referenced spec[1] does not permit the space
as a separator between the date and time portions (slthough
in some forms it may be elided altogether).
[1] http://www.omg.org/docs/ISO-stds/06-08-01.pdf
________________________________________
= Comment - Entry #16 by eikenberry on Feb 27, 2007 1:40 pm
Looks like it is mistakenly parsing the '-' dates as iso8601.
The big iso8601Match regex matches the first example below, which it shouldn't (due to the space).
Eg.
>>> DateTime('2007-02-27 13:09:01.279').timezone()
'GMT+0'
>>> DateTime('2007/02/27 13:09:01.279').timezone()
'US/Eastern'
________________________________________
= Comment - Entry #15 by bacarter on Feb 26, 2007 9:18 pm
This patch appears to break backwards API compatibility. DateTime() objects now assume any date entered is in GMT unless a timezone is specified. This is contrary to zope 2.9.5 behavior where the timezone is assumed to be the system default. The new code is also inconsistent:
print "\n>>>>>> ", DateTime().timezone()
print "\n>>>>>> ", DateTime('2006-02-27').timezone()
print "\n>>>>>> ", DateTime('2006-02-27 12:30:00').timezone()
print "\n>>>>>> ", DateTime().greaterThan(DateTime('2007-02-27 01:00:00'))
Prints:
>>>>>> US/Pacific
>>>>>> GMT+0
>>>>>> GMT+0
>>>>>> True
Note that a DateTime() with no string returns an instance in the the machine's timezone (Pacific) however all other instances are in GMT. This really screws up the final comparison, where we end up comparing a GMT time to a Pacific time.
________________________________________
= Comment - Entry #14 by tseaver on Dec 13, 2006 12:51 pm
> = Comment - Entry #13 by dtremea on Dec 13, 2006 12:19 pm
> sorry for not have fixed this on the last weekend as I promised, but my
> goal is to do it later today. In fact, I have 80% from it done, which
> means we add the ISO8601 support and *keep* the backward compatibility.
I'm sorry if my reversions today on the 2.9 and 2.10 branchs
make your merge harder. :(
> Some remarks: about the docstrings on tests, yep, I'm aware of it,
> that's why I changed them to comments (and not the other way round).
My bad -- I think I was looking at a reversed diff.
> About the cosmetical code changes, sorry for that, but I just
> standardized with the remaining code from the same file... :-)
I will assume that was my bad, too.
In general, I would avoid committing "cleanup" changes like those
at the same time as "substantive" changes, and would only commit such "cleanups" on the trunk.
________________________________________
= Comment - Entry #13 by dtremea on Dec 13, 2006 12:19 pm
Hey Tres,
sorry for not have fixed this on the last weekend as I promised, but my goal is to do it later today. In fact, I have 80% from it done, which means we add the ISO8601 support and *keep* the backward compatibility.
Some remarks: about the docstrings on tests, yep, I'm aware of it,
that's why I changed them to comments (and not the other way round).
About the cosmetical code changes, sorry for that, but I just standardized with the remaining code from the same file... :-)
________________________________________
= Comment - Entry #12 by tseaver on Dec 13, 2006 11:35 am
There is also a bunch of "gratuitous" janitorial change (e.g., removing periods at the end of doctstring sentences) which is
both debatable on the merits (An imperative sentence *ought* to
end with a period) and inappropriate for code long in maintenance
mode (it makes finding the "real" changes, or applying other
patches, needlessly difficult.
________________________________________
= Comment - Entry #11 by tseaver on Dec 13, 2006 11:32 am
Note as well that docstrings are not allowed for testcase methods:
the testrunner displays them, if present, rather than the name
of the method, which is not acceptable.
________________________________________
= Comment - Entry #10 by tseaver on Dec 13, 2006 11:31 am
This change has to be reverted on the release branches, as it
breaks semantics of peoples *persistent data* with no possibility
of correct BBB.
On the trunk, such a change should come with an *option" to enable
the new semantics; that option should be *disabled* through
at least the next release, at which point it might safely be
enabled.
I will revert on the 2.9 and 2.10 branches.
________________________________________
= Resubmit - Entry #9 by shh on Nov 23, 2006 5:03 am
Status: Resolved => Accepted
FWIW, I am all for backward compatiblity in this case.
"Naive" ISO (w/o timezone) used to mean local time. I'd be very reluctant to change this behavior, even though I agree it is "wrong". There are currently tests breaking in CMF and ATContentTypes because of this, and I am worried that there is a lot of code out there relying on the local time interpretation.
I could however imagine a zope.conf flag, say, 'datetime-strict-iso-parsing' that would be *off* by default, at least on the maintenance branches (2.9, 2.10).
________________________________________
= Comment - Entry #8 by dtremea on Nov 22, 2006 11:26 am
Looks like this fix broke 3 CMF tests:
http://mail.zope.org/pipermail/cmf-tests/2006-November/003324.html
After further investigation, I would say that "YYYY-MM-DD" dates
are in the ISO8601 format, and thus GMT based.
Dates in format "YYYY-MM-DD HH:MM:SS" are also in ISO format, but
Zope wasn't recognizing they as such. Now Zope does.
I argue that CMF was misusing DateTime, and that the new behavior
is the correct, otherwise we'll never get a consistent behavior
for the ISO dates.
I argue that the specific getBeginAndEndTimes method from
CalendarTool.py should also use the timezone, analog to what the
getEventsForThisDay method does.
Thoughts?
________________________________________
= Resolve - Entry #7 by dtremea on Nov 20, 2006 10:02 pm
Status: Accepted => Resolved
Fixed on:
- 2.9: http://svn.zope.org/Zope/branches/2.9/?rev=71230&view=rev
- 2.10: http://svn.zope.org/Zope/branches/2.10/?rev=71231&view=rev
- trunk: http://svn.zope.org/Zope/trunk/?rev=71232&view=rev
________________________________________
= Comment - Entry #6 by tuppence on Nov 7, 2006 5:52 am
I don't have time to test this new patch, but it sounds like a *considerable* improvement to mine. If others are happy, so am I!
Thanks Dorneles :).
________________________________________
= Accept - Entry #5 by dtremea on Nov 5, 2006 4:00 pm
Status: Pending => Accepted
Supporters added: dtremea
Uploaded: "DateTime-ISO8601.patch"
- http://www.zope.org/Collectors/Zope/2191/DateTime-ISO8601.patch/view
Hey folks,
> The patch is supposed to support both formats, and indeed, it
> does seem to. The test also tests both formats.
>
> The problem is that another, apparenly unrelated test, now fails
> (with details below). I'm really not sure why this separate test
> fails now. If anyone can suggest anything to try after looking
> at the failure below, I'd love to hear it.
I read the ISO8601 3rd edition standard specification[1], from 2004-12-01, and added support for these features:
- reduce date format
- week format
- day of year format
- hours/minutes fractions
- optional date/time separators
- optional seconds/minutes
- timezones with only one digit for hour
Attached is the patch with tests for each feature. Additionally,
it fixes the failing test introduced by the initial work on this
issue.
Please review it and give me a '+1', so I can commit my first
Zope code contribution!
Any feedback is welcome.
[1] http://www.omg.org/docs/ISO-stds/06-08-01.pdf
PS: Don't be afraid of that 45 lines regex, it's just for the
sake of readability and understanding... :-)
________________________________________
= Comment - Entry #4 by tuppence on Oct 15, 2006 7:55 am
The patch is supposed to support both formats, and indeed, it does seem to. The test also tests both formats.
The problem is that another, apparenly unrelated test, now fails (with details below). I'm really not sure why this separate test fails now. If anyone can suggest anything to try after looking at the failure below, I'd love to hear it.
"""
tim@teresa 2.8.8 $ bin/zopectl test --libdir lib/python/DateTime
Running tests via: /usr/bin/python /home/tim/zope/freethink/2.8.8/bin/test.py -v --config-file /home/tim/zope/freethink/2.8.8/etc/zope.conf --libdir lib/python/DateTime
Running unit tests at level 1
Running unit tests from /home/tim/zope/freethink/2.8.8/lib/python/DateTime
Parsing /home/tim/zope/freethink/2.8.8/etc/zope.conf
..............F.................
======================================================================
FAIL: testInternationalDateformat (tests.testDateTime.DateTimeTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/tim/zope/freethink/2.8.8/lib/python/DateTime/tests/testDateTime.py", line 343, in testInternationalDateformat
self.assertEqual(d_us, d_int)
File "/usr/lib/python2.3/unittest.py", line 302, in failUnlessEqual
raise self.failureException, \
AssertionError: DateTime('1990/01/02') != DateTime('1990/01/01')
----------------------------------------------------------------------
Ran 32 tests in 30.515s
FAILED (failures=1)
"""
________________________________________
= Comment - Entry #3 by ajung on Oct 15, 2006 2:09 am
ISO8601 allows both YYYY-MM-DD and YYYYMMDD. DateTime should support *both*. A related patch/unittest must support both.
________________________________________
= Comment - Entry #2 by tuppence on Sep 12, 2006 6:59 pm
Uploaded: "testdatetime.diff"
- http://www.zope.org/Collectors/Zope/2191/testdatetime.diff/view
And here is a patch that tests the new behaviour.
The problem is that the first patch uploaded here causes another test to fail. I cannot see how or why, and probing with DateTime manually from the python prompt, I'm unable to reproduce the test failure.
If anyone with a better grasp of this has any ideas, I'd love to hear them.
________________________________________
= Request - Entry #1 by tuppence on Sep 12, 2006 6:57 pm
Uploaded: "datetime.diff"
- http://www.zope.org/Collectors/Zope/2191/datetime.diff/view
The DateTime.DateTime constructor successfully parses ISO8601 strings like '2006-09-12T20:33:11Z' but does not recognise them like '20060912T20:33:11Z'. According to http://www.w3.org/TR/NOTE-datetime, the latter is not valid, but http://www.cl.cam.ac.uk/~mgk25/iso-time.html suggests otherwise. I'm not sure which is correct, but I have a problem in that Ecto (the remote blogging tool) is sending strings like the latter and I need to parse them.
I'm uploading a diff that enables the behaviour that I'm after.
==============================================================