802760 - Document all ISO-8601 date parsing extensions

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[André Bargull [:anba]]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/20100101 Firefox/16.0
Build ID: 20121010144125

Steps to reproduce:

date_parseISOString() in jsdate.cpp contains three extensions to the simplified ISO-8601 date format specified in ES5.1, all but one are undocumented. Extensions are somewhat unwanted (cf. bug 791320), so they should be documented in jsdate.cpp at least.

1. Timezone specifier without ":" -> the only documented extension (see bug 682754)
2. time-only forms -> leftover from the switch ES5 to ES5.1; not documented
3. fractional seconds -> exactly three decimal digits per ES5.1, currently implemented `at least one` decimal digit; not documented

Comment 1

[Eric Skoglund [:tephra]]

Attachment: bug 802760: Document all ISO-8601 date parsing exceptions

Update the documentation for ParseISOStyleDate to reflect the current
specification and the extensions we allow.

Review commit: https://reviewboard.mozilla.org/r/190146/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/190146/

Comment 2

[André Bargull [:anba]]

Comment on attachment 8919245 [details]
bug 802760: Document all ISO-8601 date parsing exceptions

https://reviewboard.mozilla.org/r/190146/#review195556

Thanks for taking up this bug! I've left a few comments to note which aspects can still be improved or should also be documented.

::: js/src/jsdate.cpp:789
(Diff revision 1)
> - * profile of the ISO 8601 format. Quoted here:
> - *
> - *   Any combination of the date formats with the time formats is
> - *   allowed, and also either the date or the time can be missing.
> - *
> - *   The specification is silent on the meaning when fields are
> + * of the ISO 8601 Extended Format. As per the spec omitted month and day
> + * values are defaulted to '01', omitted HH:mm:ss values are defaulted to '00'
> + * and an omitted sss field is defaulted to '000'.
> + *
> + * For cross compatibility we allow a few extensions to the Ecmascript ISO
> + * style date format.

I'd just stick with:
> For cross compatibility we allow the following extensions.

to avoid the stringing together "Ecmascript ISO style date format"

::: js/src/jsdate.cpp:798
(Diff revision 1)
> - *   missing entirely then we assume 1970-01-01 so that the time can
> - *   be aded to a date later. If the time is missing then we assume
> - *   00:00 UTC.  If the time is present but the time zone field is
> - *   missing then we use local time.
> - *
> - * For the sake of cross compatibility with other implementations we
> + *   Standalone time part:
> + *     Any of the time formats below can be parsed without a date part.
> + *     E.g "T19:00:00Z" will parse successfully. The date part will default
> + *     to 1970-01-01.
> + *
> + *   'T' from the time part can be replaces with one space:

"can be replaces" -> "may be replaced"

may sound more appropriate in this context and there's a typo in "replaced".

Also: "a space character" instead of "one space".

::: js/src/jsdate.cpp:799
(Diff revision 1)
> - *   be aded to a date later. If the time is missing then we assume
> - *   00:00 UTC.  If the time is present but the time zone field is
> - *   missing then we use local time.
> - *
> - * For the sake of cross compatibility with other implementations we
> - * make a few exceptions to the standard: months, days, hours, minutes
> + *     Any of the time formats below can be parsed without a date part.
> + *     E.g "T19:00:00Z" will parse successfully. The date part will default
> + *     to 1970-01-01.
> + *
> + *   'T' from the time part can be replaces with one space:
> + *     "1970-01-01 12:00:00Z" will parse successfully. Note that only one space

"a single space" instead of "one space" sounds better to me.

::: js/src/jsdate.cpp:800
(Diff revision 1)
> - *   00:00 UTC.  If the time is present but the time zone field is
> - *   missing then we use local time.
> - *
> - * For the sake of cross compatibility with other implementations we
> - * make a few exceptions to the standard: months, days, hours, minutes
> - * and seconds may be either one or two digits long, and the 'T' from
> + *     E.g "T19:00:00Z" will parse successfully. The date part will default
> + *     to 1970-01-01.
> + *
> + *   'T' from the time part can be replaces with one space:
> + *     "1970-01-01 12:00:00Z" will parse successfully. Note that only one space
> + *     is permitted and this is not permitted in the standalone version above.

The either-or in "months, days, hours, minutes and seconds may be __either one or two__ digits long, [...]" is no longer documented, is it? For example the spec requires exactly two digits for the month, but we also accept a single digit.

::: js/src/jsdate.cpp:802
(Diff revision 1)
> - *
> - * For the sake of cross compatibility with other implementations we
> - * make a few exceptions to the standard: months, days, hours, minutes
> - * and seconds may be either one or two digits long, and the 'T' from
> - * the time part may be replaced with a space. Given that, a date time
> - * like "1999-1-1 1:1:1" will parse successfully.
> + *
> + *   'T' from the time part can be replaces with one space:
> + *     "1970-01-01 12:00:00Z" will parse successfully. Note that only one space
> + *     is permitted and this is not permitted in the standalone version above.
> + *
> + *   One or more decimal digits for seconds:

Typo: "seconds" -> "milliseconds"

::: js/src/jsdate.cpp:803
(Diff revision 1)
> - * For the sake of cross compatibility with other implementations we
> - * make a few exceptions to the standard: months, days, hours, minutes
> - * and seconds may be either one or two digits long, and the 'T' from
> - * the time part may be replaced with a space. Given that, a date time
> - * like "1999-1-1 1:1:1" will parse successfully.
> + *   'T' from the time part can be replaces with one space:
> + *     "1970-01-01 12:00:00Z" will parse successfully. Note that only one space
> + *     is permitted and this is not permitted in the standalone version above.
> + *
> + *   One or more decimal digits for seconds:
> + *     The specification allows for exactly 3 decimal digits for the fractional

"The specification requires exactly [...]" to stress the requirement aspect.

Nit: I'd spell out "three" instead of using the numeral.

::: js/src/jsdate.cpp:804
(Diff revision 1)
> - * make a few exceptions to the standard: months, days, hours, minutes
> - * and seconds may be either one or two digits long, and the 'T' from
> - * the time part may be replaced with a space. Given that, a date time
> - * like "1999-1-1 1:1:1" will parse successfully.
> + *     "1970-01-01 12:00:00Z" will parse successfully. Note that only one space
> + *     is permitted and this is not permitted in the standalone version above.
> + *
> + *   One or more decimal digits for seconds:
> + *     The specification allows for exactly 3 decimal digits for the fractional
> + *     second part. We allow 1 or more digits either padding or clipping the fractional

Same here, "one" instead of "1".

::: js/src/jsdate.cpp:804
(Diff revision 1)
> - * make a few exceptions to the standard: months, days, hours, minutes
> - * and seconds may be either one or two digits long, and the 'T' from
> - * the time part may be replaced with a space. Given that, a date time
> - * like "1999-1-1 1:1:1" will parse successfully.
> + *     "1970-01-01 12:00:00Z" will parse successfully. Note that only one space
> + *     is permitted and this is not permitted in the standalone version above.
> + *
> + *   One or more decimal digits for seconds:
> + *     The specification allows for exactly 3 decimal digits for the fractional
> + *     second part. We allow 1 or more digits either padding or clipping the fractional

Unfortunately clipping doesn't quite match our current implementation (bug 746529), but I don't really know how to describe the current implementation using only a few words, because I really don't want to mention IEEE 754 rounding here. :-)

::: js/src/jsdate.cpp:804
(Diff revision 1)
> - * make a few exceptions to the standard: months, days, hours, minutes
> - * and seconds may be either one or two digits long, and the 'T' from
> - * the time part may be replaced with a space. Given that, a date time
> - * like "1999-1-1 1:1:1" will parse successfully.
> + *     "1970-01-01 12:00:00Z" will parse successfully. Note that only one space
> + *     is permitted and this is not permitted in the standalone version above.
> + *
> + *   One or more decimal digits for seconds:
> + *     The specification allows for exactly 3 decimal digits for the fractional
> + *     second part. We allow 1 or more digits either padding or clipping the fractional

The line exceeds 80 characters, please break it up into two lines:

https://wiki.mozilla.org/JavaScript:SpiderMonkey:C%2B%2B_Coding_Style#Other_whitespace

::: js/src/jsdate.cpp:805
(Diff revision 1)
> - * and seconds may be either one or two digits long, and the 'T' from
> - * the time part may be replaced with a space. Given that, a date time
> - * like "1999-1-1 1:1:1" will parse successfully.
> + *     is permitted and this is not permitted in the standalone version above.
> + *
> + *   One or more decimal digits for seconds:
> + *     The specification allows for exactly 3 decimal digits for the fractional
> + *     second part. We allow 1 or more digits either padding or clipping the fractional
> + *     part. E.g a time part with "T19:00:00.1" will parse as "T19:00.00.100" and

Nit: "E.g." (dot after g)

::: js/src/jsdate.cpp:808
(Diff revision 1)
> + *     The specification allows for exactly 3 decimal digits for the fractional
> + *     second part. We allow 1 or more digits either padding or clipping the fractional
> + *     part. E.g a time part with "T19:00:00.1" will parse as "T19:00.00.100" and
> + *     "T19:00:00.9999" will parse as "T19:00:00.999".
> + *
> + *   Timezone specifier without ':':

Nit: "Time zone" instead of "Timezone".

::: js/src/jsdate.cpp:809
(Diff revision 1)
> + *     second part. We allow 1 or more digits either padding or clipping the fractional
> + *     part. E.g a time part with "T19:00:00.1" will parse as "T19:00.00.100" and
> + *     "T19:00:00.9999" will parse as "T19:00:00.999".
> + *
> + *   Timezone specifier without ':':
> + *     We allow the timezone to be specified without a ':'. E.g "T19:00:00+0700" is

Also "E.g." here.

Comment 3

[Eric Skoglund [:tephra]]

Comment on attachment 8919245 [details]
bug 802760: Document all ISO-8601 date parsing exceptions

https://reviewboard.mozilla.org/r/190146/#review195556

> The either-or in "months, days, hours, minutes and seconds may be __either one or two__ digits long, [...]" is no longer documented, is it? For example the spec requires exactly two digits for the month, but we also accept a single digit.

Missed this one. We have documented it in the end of the comment (after the "where:" part) but not as a extension. Will add a section about this.

> Unfortunately clipping doesn't quite match our current implementation (bug 746529), but I don't really know how to describe the current implementation using only a few words, because I really don't want to mention IEEE 754 rounding here. :-)

Oh I missed that bug. Two ideas spring to mind:

1. Just mention that we allow for more than one digit
2. Same as above but reference the bug (hoping to get to that bug soon) and then change to reflect when the bug is fixed

Not sure what the best course of action would be.

Comment 4

[Eric Skoglund [:tephra]]

Comment on attachment 8919245 [details]
bug 802760: Document all ISO-8601 date parsing exceptions

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190146/diff/1-2/

Comment 5

[André Bargull [:anba]]

(In reply to Eric Skoglund [:tephra] from comment #3)
> > Unfortunately clipping doesn't quite match our current implementation (bug 746529), but I don't really know how to describe the current implementation using only a few words, because I really don't want to mention IEEE 754 rounding here. :-)
> 
> Oh I missed that bug. Two ideas spring to mind:
> 
> 1. Just mention that we allow for more than one digit
> 2. Same as above but reference the bug (hoping to get to that bug soon) and
> then change to reflect when the bug is fixed
> 
> Not sure what the best course of action would be.

I don't have any preference at this point, so you're free to pick what ever is easier for you and requires the least amount of time (because I really don't think it's worthwhile to spend too much time on this one). :-)

Comment 6

[André Bargull [:anba]]

Comment on attachment 8919245 [details]
bug 802760: Document all ISO-8601 date parsing exceptions

https://reviewboard.mozilla.org/r/190146/#review196564

Looks good to me. Thanks for cleaning this up! :-)

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Autoland can't push this until all pending issues in MozReview are marked as resolved.

Comment 8

[Eric Skoglund [:tephra]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #7)
> Autoland can't push this until all pending issues in MozReview are marked as
> resolved.

Hey Ryan, sorry it's been a while since I did some patches. I've resolved all issues now.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Unfortunately, this also needs r+ from someone with Level 3 commit access in order for autoland to push it. Sorry for the hassle :(
http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/autoland.html#landing-commits

Comment 10

[André Bargull [:anba]]

Ah dang it, forgot about that autoland-only restriction.

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8919245 [details]
bug 802760: Document all ISO-8601 date parsing exceptions

https://reviewboard.mozilla.org/r/190146/#review197392

rubberstamping for :anba

Comment 12

[André Bargull [:anba]]

Thanks :gandalf! :-)

Comment 13

[Eric Skoglund [:tephra]]

Thanks for the help team! :)

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b200a66205a2
Document all ISO-8601 date parsing exceptions r=anba,gandalf

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/b200a66205a2