Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(83075)

#15873: "datetime" cannot parse ISO 8601 dates and times

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 11 months ago by nagle
Modified:
3 years ago
Reviewers:
ghost.adh, deronnax, victor.stinner, vadmium+py, ghost.adh
CC:
barry, jcea, cben_redhat.com, roysmith, Nick Coghlan, sasha, nagle_users.sourceforge.net, haypo, jwilk_jwilk.net, mcepl, eric.araujo, Arfrever, r.david.murray, dav01_myths.ru, cvrebert, karlcow, SilentGhost, Elvis.Pranskevichus, pederick_gmail.com, flying sheep, mihai_mihaic.ro, aymeric.augustin, someuniquename_gmail.com, berkerpeksag, Martin Panter, piotr.dobrogost, kirpit_gmail.com, boxed_killingar.net, jstasiak, Eric.Hanchrow, deronnax, pbryan_anode.ca, p.ganssle_gmail.com, sirex, loveneet.singh_redblink.com, dan_djcentric.com
Visibility:
Public.

Patch Set 1 #

Patch Set 2 #

Patch Set 3 #

Total comments: 2

Patch Set 4 #

Patch Set 5 #

Total comments: 12

Patch Set 6 #

Patch Set 7 #

Patch Set 8 #

Total comments: 18

Patch Set 9 #

Patch Set 10 #

Total comments: 10

Patch Set 11 #

Patch Set 12 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Lib/datetime.py View 1 2 3 4 5 6 7 8 9 10 11 9 chunks +55 lines, -0 lines 0 comments Download
Lib/test/datetimetester.py View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +61 lines, -0 lines 0 comments Download

Messages

Total messages: 9
ghost.adh_gmail.com
https://bugs.python.org/review/15873/diff/16581/Lib/datetime.py File Lib/datetime.py (right): https://bugs.python.org/review/15873/diff/16581/Lib/datetime.py#newcode153 Lib/datetime.py:153: r'(?::(?P<second>\d{1,2})(?:\.(?P<microsecond>\d{1,6})\d{0,6})?)?' You're not testing the case with more than ...
3 years, 6 months ago #1
deronnax
On 2016/02/14 14:07:32, SilentGhost wrote: > https://bugs.python.org/review/15873/diff/16581/Lib/datetime.py > File Lib/datetime.py (right): > > https://bugs.python.org/review/15873/diff/16581/Lib/datetime.py#newcode153 > ...
3 years, 6 months ago #2
victor.stinner_gmail.com
http://bugs.python.org/review/15873/diff/16583/Lib/datetime.py File Lib/datetime.py (right): http://bugs.python.org/review/15873/diff/16583/Lib/datetime.py#newcode1438 Lib/datetime.py:1438: kw['microsecond'] = int(us * 1e6) I suggest: kw['microsecond'] = ...
3 years, 6 months ago #3
Martin Panter
https://bugs.python.org/review/15873/diff/16583/Lib/datetime.py File Lib/datetime.py (right): https://bugs.python.org/review/15873/diff/16583/Lib/datetime.py#newcode150 Lib/datetime.py:150: datetime_re = re.compile( IMO this would be better moved ...
3 years, 6 months ago #4
Martin Panter
http://bugs.python.org/review/15873/diff/16583/Lib/datetime.py File Lib/datetime.py (right): http://bugs.python.org/review/15873/diff/16583/Lib/datetime.py#newcode155 Lib/datetime.py:155: ) Should probably add case-insensitive flag to allow lower-case ...
3 years, 6 months ago #5
Martin Panter
https://bugs.python.org/review/15873/diff/16594/Lib/datetime.py File Lib/datetime.py (right): https://bugs.python.org/review/15873/diff/16594/Lib/datetime.py#newcode724 Lib/datetime.py:724: """Constructs a date from a RFC 3339 string, a ...
3 years, 6 months ago #6
ghost.adh_gmail.com
https://bugs.python.org/review/15873/diff/16594/Lib/datetime.py File Lib/datetime.py (right): https://bugs.python.org/review/15873/diff/16594/Lib/datetime.py#newcode730 Lib/datetime.py:730: raise ValueError('invalid RFC 3339 date string: "%s"' % date_string) ...
3 years, 6 months ago #7
SilentGhost
The review for the patch that I think should go ahead. https://bugs.python.org/review/15873/diff/18066/Lib/datetime.py File Lib/datetime.py (right): ...
3 years ago #8
deronnax
3 years ago #9
addresses all concerns of SilentGhost except the "optional timezone minutes
part", I couldn't spot in the spec where it's said it's optional

https://bugs.python.org/review/15873/diff/16583/Lib/datetime.py
File Lib/datetime.py (right):

https://bugs.python.org/review/15873/diff/16583/Lib/datetime.py#newcode1438
Lib/datetime.py:1438: kw['microsecond'] = int(us * 1e6)
On 2016/02/15 17:29:59, haypo wrote:
> I suggest:
> 
> kw['microsecond'] = round(float(us) / 1e6)

Doesn't work, make the tests fails, microseconds always equal zero after that

https://bugs.python.org/review/15873/diff/16594/Lib/datetime.py
File Lib/datetime.py (right):

https://bugs.python.org/review/15873/diff/16594/Lib/datetime.py#newcode724
Lib/datetime.py:724: """Constructs a date from a RFC 3339 string, a strict
subset of ISO 8601
On 2016/02/18 02:45:42, vadmium wrote:
> an RFC 3339 string

Done.

https://bugs.python.org/review/15873/diff/16594/Lib/datetime.py#newcode726
Lib/datetime.py:726: raise ValueError in case of ill formatted or invalid
string.
On 2016/02/18 02:45:42, vadmium wrote:
> Capital R for Raise
> Hyphen for ill-formatted

Done.

https://bugs.python.org/review/15873/diff/16594/Lib/datetime.py#newcode730
Lib/datetime.py:730: raise ValueError('invalid RFC 3339 date string: "%s"' %
date_string)
On 2016/02/18 04:13:13, SilentGhost wrote:
> That would be better as %r (and without double-quotes). Same applies for
> equivalent code below.

Done.

https://bugs.python.org/review/15873/diff/16594/Lib/datetime.py#newcode1080
Lib/datetime.py:1080: def _parse_isotime(reg, isostring, objekt):
On 2016/02/18 02:45:42, vadmium wrote:
> Spelling: object (or maybe class_name if you don’t want to mask the builtin)

Done.

https://bugs.python.org/review/15873/diff/16594/Lib/datetime.py#newcode1089
Lib/datetime.py:1089: offset_hours, offset_mins = tzinfo[1:].split(':')
On 2016/02/18 04:13:13, SilentGhost wrote:
> .partition would be more appropriate here.

Done.

https://bugs.python.org/review/15873/diff/16594/Lib/datetime.py#newcode1099
Lib/datetime.py:1099: kw['tzinfo'] = tzinfo
On 2016/02/18 04:13:13, SilentGhost wrote:
> If you're to make this assignment conditional on tzinfo not being None, I
> believe this function would be able to accommodate date string parsing and
> therefore could be made top-level, rather than a static method of time. It
also
> could then be possible to collect all regex into a single dispatching dict,
> keyed by class, all fromisoformat functions could then all be: 
> 
> return _parse_isodatetime(cls, _string)

But the "if tzinfo" and if kw.pop('microsecond') will be uselessly evaluated
each time a date parsing is invoked, that's a gratuitous performance penaly for
a date parsing user. If you're sure it's worth it, I do it.

https://bugs.python.org/review/15873/diff/16594/Lib/datetime.py#newcode1107
Lib/datetime.py:1107: raise ValueError in case of ill formatted or invalid
string
On 2016/02/18 02:45:42, vadmium wrote:
> Missing full stop (.) at the end.

Done.

https://bugs.python.org/review/15873/diff/16594/Lib/datetime.py#newcode1367
Lib/datetime.py:1367: _DATETIME_RE = re.compile(_DATE_RE.pattern[:-1] + r'[T ]'
+ _TIME_RE.pattern,
On 2016/02/18 04:13:13, SilentGhost wrote:
> raw string literal is not needed here
It was for consistency with the other regex, when they were grouped together,
and I like to keep it for a regex string even when not needed. Do you mind ?

https://bugs.python.org/review/15873/diff/16594/Lib/datetime.py#newcode2198
Lib/datetime.py:2198: raise ImportError
On 2016/02/18 04:13:13, SilentGhost wrote:
> This line is from your local setup, I believe.
Yes. I usually remove it before patch generation, I will pay extra attention
next time.

https://bugs.python.org/review/15873/diff/18066/Lib/datetime.py
File Lib/datetime.py (right):

https://bugs.python.org/review/15873/diff/18066/Lib/datetime.py#newcode338
Lib/datetime.py:338: raise ValueError("invalid RFC 3339 %s string: %r" %
(cls.__name__, isostring))
On 2016/08/05 13:22:36, SilentGhost wrote:
> Please, wrap this line to 80 chars.

done

https://bugs.python.org/review/15873/diff/18066/Lib/datetime.py#newcode350
Lib/datetime.py:350: kw = {k: int(v) for k, v in kw.items() if v is not None}
On 2016/08/05 13:22:36, SilentGhost wrote:
> It shouldn't be possible that v would be None at this stage.

you're right

https://bugs.python.org/review/15873/diff/18066/Lib/datetime.py#newcode1059
Lib/datetime.py:1059:
r'(?P<microsecond>\.\d*)?(?P<tzinfo>Z|([+-]\d{2}:\d{2}))?$',
On 2016/08/05 13:22:36, SilentGhost wrote:
> The same issue as in alternative patch: \d* matches no digits. This afterwards
> blows up on microsecond conversion to float. Also, the minute part of the
offset
> seems to be optional according to the RFC, are you sure you put parenthesis
> correctly around numeric offset?
Oh yeah, well seen. Fixed. I added a test to check this.
Could you point where you read that the minute part is optional ? I only see in
section 5.6 that it's mandatory.

https://bugs.python.org/review/15873/diff/18066/Lib/datetime.py#newcode1415
Lib/datetime.py:1415: _isore = re.compile(_DATE_RE.pattern[:-1] + r'[T ]' +
_TIME_RE.pattern,
On 2016/08/05 13:22:36, SilentGhost wrote:
> Can't you reach regexes as class attributes of date and time? Then there won't
> be any need for globals and their subsequent deletion.

It was to somewhat reduce coupling, but that was probably pointless

https://bugs.python.org/review/15873/diff/18066/Lib/datetime.py#newcode1550
Lib/datetime.py:1550: def fromisoformat(cls, date_string):
On 2016/08/05 13:22:36, SilentGhost wrote:
> Since datetime is a subclass of date, I think this method is not required.

you're right
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7+