improve DateTime parsing a UTC timezone offset

improve DateTime parsing a UTC timezone offset

BUG= http://dartbug.com/14550
R=floitsch@google.com

Committed: https://code.google.com/p/dart/source/detail?r=29615

[nikgraf, 2013-10-28 22:47:09]

Hey guys,

This is a small improvement to allow parsing Strings with a zero offset.

see issue https://code.google.com/p/dart/issues/detail?id=14550

[floitsch, 2013-10-28 23:01:24]

LGTM with comments.

https://codereview.chromium.org/48823005/diff/1/sdk/lib/core/date_time.dart
File sdk/lib/core/date_time.dart (right):

https://codereview.chromium.org/48823005/diff/1/sdk/lib/core/date_time.dart#n...
sdk/lib/core/date_time.dart:209: r'(?:[
T](\d\d)(?::?(\d\d)(?::?(\d\d)(.\d{1,6})?)?)? ?([zZ]|(\+00(:?00)?)?))?$');
80 chars.
The "(\+00" should be a non-capturing ("(:?").

[nikgraf, 2013-10-28 23:37:33]

On 2013/10/28 23:01:24, floitsch wrote:
> LGTM with comments.
> 
> https://codereview.chromium.org/48823005/diff/1/sdk/lib/core/date_time.dart
> File sdk/lib/core/date_time.dart (right):
> 
>
https://codereview.chromium.org/48823005/diff/1/sdk/lib/core/date_time.dart#n...
> sdk/lib/core/date_time.dart:209: r'(?:[
> T](\d\d)(?::?(\d\d)(?::?(\d\d)(.\d{1,6})?)?)? ?([zZ]|(\+00(:?00)?)?))?$');
> 80 chars.
> The "(\+00" should be a non-capturing ("(:?").

+1

Just improved the regular expression.

[Lasse Reichstein Nielsen, 2013-10-29 08:48:45]

https://codereview.chromium.org/48823005/diff/60001/sdk/lib/core/date_time.dart
File sdk/lib/core/date_time.dart (right):

https://codereview.chromium.org/48823005/diff/60001/sdk/lib/core/date_time.da...
sdk/lib/core/date_time.dart:210: r' ?([zZ]|(?:\+00(?::?00)?)?))?$'); // The
timezone part
This seemns to accept a single terminating space (from the beginning of the
timezone part, even if the timezone part is omitted).
Maybe wrap that in (?:...) too, so the '?' for the timezone part includes the
space.

That is, a grammar something like:

  date ::= yeardate time_opt timezone_opt
  yeardate ::= year colon_opt month colon_opt day
  year ::= sign_opt digit{4,5}
  colon_opt :: <empty> | ':'
  sign_opt ::=  <empty> | '+' | '-'
  month ::= digit{2}
  day ::= digit{2}
  time_opt ::= <empty> | (' ' | 'T') hour minutes_opt
  minutes_opt ::= <empty> | ':' digit{2} seconds_opt
  seconds_opt ::= <empty> | ':' digit{2} millis_opt
  millis_opt ::= <empty> | '.' digit{1,6}
  timezone_opt ::= <empty> | space_opt timezone
  space_opt :: ' ' | <empty>
  timezone ::= 'z' | 'Z' | '+' '0' '0' timezonemins_opt
  timezonemins_opt ::= <empty> | colon_opt '0' '0'
  
(Does this look right?)

It's a pet peeve of mine to not create complex regexps without a specification
of what it should do. It's hard enough to read a regexp to see whether it does
the correct thing, it's pretty much impossible if you don't even know what
correct is :).

[floitsch, 2013-10-30 23:17:34]

https://codereview.chromium.org/48823005/diff/60001/sdk/lib/core/date_time.dart
File sdk/lib/core/date_time.dart (right):

https://codereview.chromium.org/48823005/diff/60001/sdk/lib/core/date_time.da...
sdk/lib/core/date_time.dart:210: r' ?([zZ]|(?:\+00(?::?00)?)?))?$'); // The
timezone part
On 2013/10/29 08:48:45, Lasse Reichstein Nielsen wrote:
> This seemns to accept a single terminating space (from the beginning of the
> timezone part, even if the timezone part is omitted).
> Maybe wrap that in (?:...) too, so the '?' for the timezone part includes the
> space.
> 
> That is, a grammar something like:
> 
>   date ::= yeardate time_opt timezone_opt
>   yeardate ::= year colon_opt month colon_opt day
>   year ::= sign_opt digit{4,5}
>   colon_opt :: <empty> | ':'
>   sign_opt ::=  <empty> | '+' | '-'
>   month ::= digit{2}
>   day ::= digit{2}
>   time_opt ::= <empty> | (' ' | 'T') hour minutes_opt
>   minutes_opt ::= <empty> | ':' digit{2} seconds_opt
>   seconds_opt ::= <empty> | ':' digit{2} millis_opt
>   millis_opt ::= <empty> | '.' digit{1,6}
>   timezone_opt ::= <empty> | space_opt timezone
>   space_opt :: ' ' | <empty>
>   timezone ::= 'z' | 'Z' | '+' '0' '0' timezonemins_opt
>   timezonemins_opt ::= <empty> | colon_opt '0' '0'
>   
> (Does this look right?)
> 
> It's a pet peeve of mine to not create complex regexps without a specification
> of what it should do. It's hard enough to read a regexp to see whether it does
> the correct thing, it's pretty much impossible if you don't even know what
> correct is :).

Done.

[floitsch, 2013-10-31 00:53:27]

Committed patchset #4 manually as r29615 (presubmit successful).

[nikgraf, 2013-10-31 03:15:22]

On 2013/10/31 00:53:27, floitsch wrote:
> Committed patchset #4 manually as r29615 (presubmit successful).

Great, thanks guys.