Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Why do toString methods truncate? #329

Closed
gibson042 opened this issue Jan 25, 2020 · 14 comments · Fixed by #1006
Closed

Why do toString methods truncate? #329

gibson042 opened this issue Jan 25, 2020 · 14 comments · Fixed by #1006
Assignees
Labels
behavior Relating to behavior defined in the proposal documentation Additions to documentation non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved

Comments

@gibson042
Copy link
Collaborator

gibson042 commented Jan 25, 2020

Date.prototype.toISOString always returns a serialization at full resolution including all time elements, but Temporal.DateTime.prototype.toString and Temporal.Time.prototype.toString truncate trailing zeroes. This inconsistent output can disrupt the readability of list output, and even break sorting:

  1. 1970-01-01T00:00:00.000100Z
  2. 1970-01-01T00:00:00.010Z
  3. 1970-01-01T00:00:01Z
  4. 1970-01-01T00:00Z ⚠️
  5. 1970-01-01T00:01:40Z

Is there a benefit that justifies this behavior?

@gibson042 gibson042 added question behavior Relating to behavior defined in the proposal labels Jan 25, 2020
@ferk6a
Copy link

ferk6a commented Jan 27, 2020

I'll add that js-joda (which is just a port of JSR-310, Java's newer date/time implementation which is similar to Temporal) does the same:

> require('@js-joda/core').ZonedDateTime.parse('1970-01-01T00:00:00.000Z').toString()
'1970-01-01T00:00Z'

My guess is that since seconds are optional by the standard, they strive for the most compact representation, but it does annoy me that it breaks sorting and a lot of incomplete ISO date parsing implementations.

One example is SQL Server which doesn't accept such strings as proper dates, and debugging this could be a bit of a nightmare depending on the situation.

@littledan littledan added this to the Stage 3 milestone May 14, 2020
@pipobscure
Copy link
Collaborator

Discussion Summary: we need both behaviours that are chosen via a parameter that gives the smallest unit or this behaviour if none.

Needs more Discussion

@littledan
Copy link
Member

I think we can agree that the truncated form is also valuable. The idea would be to consider an API (like an options bag as an argument) to invoke a less-truncated mode, e.g., if you want the width of a bunch of strings to be the same. We can discuss this more, but since this is additive and small, it doesn't block the polyfill from being shipped.

@ptomato
Copy link
Collaborator

ptomato commented Aug 12, 2020

One proposal would be to add an options argument to Temporal.DateTime.prototype.toString(), Temporal.Absolute.prototype.toString(), and Temporal.Time.prototype.toString(), with { sortable: true } or { compact: true } depending on which aspect we wanted to emphasize.

However, it's well known by now that I don't like proliferation of options. 😄 So, another proposal would be to always output the seconds, but continue omitting trailing zeros (and the decimal point, if applicable) in the fractional part. (Omitting the seconds is the only thing that breaks sorting, I think?)

I think we can agree that the truncated form is also valuable.

Can you elaborate? Is it for readability? If so, wouldn't it be better to use toLocaleString() for readability?

@justingrant
Copy link
Collaborator

Instead of new options, why not simply offer the same rounding options (smallestUnit and whatever Intl will call the rounding mode option) on toString()? With a default of {smallestUnit: 'nanoseconds'} there's no rounding by default so we'd show all 9 decimal digits.

This may address any readability concerns as well as provide rounding as well as truncation. And it'd allow fine-grained control over precision for interop with other systems that can't handle nanoseconds.

@justingrant
Copy link
Collaborator

BTW, I added the proposed solution above to #827. Feedback appreciated!

@justingrant
Copy link
Collaborator

justingrant commented Aug 19, 2020

FWIW, JSCalendar (the JSON counterpart to iCalendar/RFC5545) has this to say re: truncation:

1.4.3. UTCDateTime

This is a string in [RFC3339] "date-time" format, with the further
restrictions that any letters MUST be in uppercase, the time
component MUST be included and the time offset MUST be the character
"Z". Fractional second values MUST NOT be included unless non-zero
and MUST NOT have trailing zeros, to ensure there is only a single
representation for each date-time.

For example "2010-10-10T10:10:10.003Z" is OK, but
"2010-10-10T10:10:10.000Z" is invalid and is correctly encoded as
"2010-10-10T10:10:10Z".

@justingrant
Copy link
Collaborator

Per 2020-08-28 meeting, we removed toString requirements from #827 and moved them here. Below was the content from #827. The basic idea was to use smallestUnit to control which units are shown and what precision is shown for sub-second units. We don't have to use the approach below-- just capturing it here in case it'll be useful.

  • 8.2.1 If smallestUnit is 'nanosecond' then there is no change to the default output format.
  • 8.2.1 If smallestUnit is 'microsecond' then the seconds in the result should be shown with 6 decimal digits
  • 8.2.3 If smallestUnit is 'millisecond' then the seconds in the result should be shown with 3 decimal digits
  • 8.2.4 If smallestUnit is 'second' then the seconds should be shown in the result with no decimal part.
  • 8.2.5 If smallestUnit is 'minute' then the seconds should not be shown in the result at all, e.g. 2020-02-28T12:30.
  • 8.2.6 If smallestUnit is 'hour' then the seconds should be shown as zero with no seconds shown, e.g. 2020-02-28T12:00.
  • 8.2.7 If smallestUnit is 'day' then the time portion should not be included, e.g. 2020-02-28.
  • 8.2.8 If smallestUnit is 'week' then a RangeError should be thrown, for the same reasons as in round().
  • 8.2.9 If smallestUnit is 'month' then day should be set to 1, e.g. 2020-02-01.
  • 8.2.10 If smallestUnit is 'year' then month and day should be set to 1, e.g. 2020-01-01.

@justingrant
Copy link
Collaborator

Also, adding some use cases removed from #827:

  • Get an ISO string for a DateTime that does not include seconds or smaller units. Round the result to the nearest second.
  • Get an ISO string for a LocalDateTime that only shows three digits of sub-second precision, rounding to the nearest millisecond. This will be suitable for parsing into other types that only handle millisecond precision.

@ptomato
Copy link
Collaborator

ptomato commented Sep 18, 2020

Meeting, Sept. 18: We agree that we want to be able to set the output precision in any case, so we do want this, but we need a proposal on the table for how the option actually works.

@justingrant
Copy link
Collaborator

Did someone volunteer to build this proposal? There are only a handful of remaining open issues affecting API shape, so does it make sense to assign owners to each one so they don't get lost and do get prompt attention?

@pipobscure
Copy link
Collaborator

pipobscure commented Sep 18, 2020 via email

@ptomato
Copy link
Collaborator

ptomato commented Oct 8, 2020

This proposal is now described in #703 and has only one open question left at the time of writing. The precision part can be implemented separately from the rest, so maybe it's a good idea to keep this issue open and do the precision part first.

@ptomato
Copy link
Collaborator

ptomato commented Oct 9, 2020

Here's what's ready to be implemented on this issue, the parts of #703 that don't depend on ZonedDateTime:

  • 2.4 smallestUnit - Don't emit any unit smaller than this one.
    • 2.4.1 Limited to the units that are 'minutes' or smaller because minutes are required to be a valid ISO string but smaller units are optional.
  • 2.5 roundingMode - controls rounding, works same as round()
    • 2.5.1 Default is 'trunc'
  • 2.6 roundingIncrement will not be supported because it's not core to the output use case.
  • 2.7 fractionalSecondDigits: control the number of fractional digits output for the seconds unit.
    • 2.7.1 Choices are: 'auto' | 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9.
    • 2.7.2 Default depends on smallestUnit, but can be overridden
      • 2.7.2.1 'minutes': n/a (no digits are shown, so fractionalSecondDigits is ignored)
      • 2.7.2.2 'seconds': 0
      • 2.7.2.3 'milliseconds': 3
      • 2.7.2.4 'microseconds': 6
      • 2.7.2.5 'nanoseconds': 9
      • 2.7.2.6 undefined: 'auto'
    • 2.7.3 'auto' means to omit trailing zeroes in fractional seconds. Seconds are always displayed.

@ptomato ptomato added documentation Additions to documentation non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved labels Oct 9, 2020
ptomato added a commit that referenced this issue Oct 16, 2020
This will be reused in Temporal.Instant.prototype.toString when we add
precision options.

See: #329
ptomato added a commit that referenced this issue Oct 16, 2020
On DateTime.toString(), Time.toString(), and Instant.toString(), we add
an options bag with the options `fractionalSecondDigits`, `smallestUnit`,
and `roundingMode`. `smallestUnit` and `fractionalSecondDigits` control
which units are output, and `roundingMode` is a rarely-needed option that
allows other rounding behaviour than truncation.

The default behaviour is changed as well. Previously seconds would not be
output if the seconds and lower components would be 0, and the precision
after the decimal point would be 0, 3, 6, or 9. After this change, seconds
are always output, and all trailing zeroes after the decimal point are
dropped.

This change requires a lot of adjustments to expected test output.

Closes: #329
@ptomato ptomato self-assigned this Oct 16, 2020
Ms2ger pushed a commit that referenced this issue Oct 19, 2020
In toString() we are going to have a different default rounding mode than
in round() and difference(), so don't hardcode the default in the abstract
operation.

See: #329
Ms2ger pushed a commit that referenced this issue Oct 19, 2020
This will be reused in Temporal.DateTime.prototype.toString when we add
precision options.

See: #329
Ms2ger pushed a commit that referenced this issue Oct 19, 2020
This will be reused in Temporal.Instant.prototype.toString when we add
precision options.

See: #329
Ms2ger pushed a commit that referenced this issue Oct 19, 2020
On DateTime.toString(), Time.toString(), and Instant.toString(), we add
an options bag with the options `fractionalSecondDigits`, `smallestUnit`,
and `roundingMode`. `smallestUnit` and `fractionalSecondDigits` control
which units are output, and `roundingMode` is a rarely-needed option that
allows other rounding behaviour than truncation.

The default behaviour is changed as well. Previously seconds would not be
output if the seconds and lower components would be 0, and the precision
after the decimal point would be 0, 3, 6, or 9. After this change, seconds
are always output, and all trailing zeroes after the decimal point are
dropped.

This change requires a lot of adjustments to expected test output.

Closes: #329
ptomato added a commit that referenced this issue Oct 19, 2020
In toString() we are going to have a different default rounding mode than
in round() and difference(), so don't hardcode the default in the abstract
operation.

See: #329
ptomato added a commit that referenced this issue Oct 19, 2020
This will be reused in Temporal.DateTime.prototype.toString when we add
precision options.

See: #329
ptomato added a commit that referenced this issue Oct 19, 2020
This will be reused in Temporal.Instant.prototype.toString when we add
precision options.

See: #329
ptomato added a commit that referenced this issue Oct 19, 2020
On DateTime.toString(), Time.toString(), and Instant.toString(), we add
an options bag with the options `fractionalSecondDigits`, `smallestUnit`,
and `roundingMode`. `smallestUnit` and `fractionalSecondDigits` control
which units are output, and `roundingMode` is a rarely-needed option that
allows other rounding behaviour than truncation.

The default behaviour is changed as well. Previously seconds would not be
output if the seconds and lower components would be 0, and the precision
after the decimal point would be 0, 3, 6, or 9. After this change, seconds
are always output, and all trailing zeroes after the decimal point are
dropped.

This change requires a lot of adjustments to expected test output.

Closes: #329
ptomato added a commit that referenced this issue Oct 20, 2020
In toString() we are going to have a different default rounding mode than
in round() and difference(), so don't hardcode the default in the abstract
operation.

See: #329
ptomato added a commit that referenced this issue Oct 20, 2020
This will be reused in Temporal.DateTime.prototype.toString when we add
precision options.

See: #329
ptomato added a commit that referenced this issue Oct 20, 2020
This will be reused in Temporal.Instant.prototype.toString when we add
precision options.

See: #329
ptomato added a commit that referenced this issue Oct 20, 2020
On DateTime.toString(), Time.toString(), and Instant.toString(), we add
an options bag with the options `fractionalSecondDigits`, `smallestUnit`,
and `roundingMode`. `smallestUnit` and `fractionalSecondDigits` control
which units are output, and `roundingMode` is a rarely-needed option that
allows other rounding behaviour than truncation.

The default behaviour is changed as well. Previously seconds would not be
output if the seconds and lower components would be 0, and the precision
after the decimal point would be 0, 3, 6, or 9. After this change, seconds
are always output, and all trailing zeroes after the decimal point are
dropped.

This change requires a lot of adjustments to expected test output.

Closes: #329
ptomato added a commit that referenced this issue Oct 22, 2020
In toString() we are going to have a different default rounding mode than
in round() and difference(), so don't hardcode the default in the abstract
operation.

See: #329
ptomato added a commit that referenced this issue Oct 22, 2020
This will be reused in Temporal.DateTime.prototype.toString when we add
precision options.

See: #329
ptomato added a commit that referenced this issue Oct 22, 2020
This will be reused in Temporal.Instant.prototype.toString when we add
precision options.

See: #329
ptomato added a commit that referenced this issue Oct 22, 2020
On DateTime.toString(), Time.toString(), and Instant.toString(), we add
an options bag with the options `fractionalSecondDigits`, `smallestUnit`,
and `roundingMode`. `smallestUnit` and `fractionalSecondDigits` control
which units are output, and `roundingMode` is a rarely-needed option that
allows other rounding behaviour than truncation.

The default behaviour is changed as well. Previously seconds would not be
output if the seconds and lower components would be 0, and the precision
after the decimal point would be 0, 3, 6, or 9. After this change, seconds
are always output, and all trailing zeroes after the decimal point are
dropped.

This change requires a lot of adjustments to expected test output.

Closes: #329
ptomato added a commit that referenced this issue Oct 22, 2020
In toString() we are going to have a different default rounding mode than
in round() and difference(), so don't hardcode the default in the abstract
operation.

See: #329
ptomato added a commit that referenced this issue Oct 22, 2020
This will be reused in Temporal.DateTime.prototype.toString when we add
precision options.

See: #329
ptomato added a commit that referenced this issue Oct 22, 2020
This will be reused in Temporal.Instant.prototype.toString when we add
precision options.

See: #329
ptomato added a commit that referenced this issue Oct 22, 2020
On DateTime.toString(), Time.toString(), and Instant.toString(), we add
an options bag with the options `fractionalSecondDigits`, `smallestUnit`,
and `roundingMode`. `smallestUnit` and `fractionalSecondDigits` control
which units are output, and `roundingMode` is a rarely-needed option that
allows other rounding behaviour than truncation.

The default behaviour is changed as well. Previously seconds would not be
output if the seconds and lower components would be 0, and the precision
after the decimal point would be 0, 3, 6, or 9. After this change, seconds
are always output, and all trailing zeroes after the decimal point are
dropped.

This change requires a lot of adjustments to expected test output.

Closes: #329
ptomato added a commit that referenced this issue Jan 12, 2021
I overlooked Duration in #329. This adds the same options except that the
values for smallestUnit are preferred to be plural, and 'minutes' is not
allowed.
Ms2ger pushed a commit that referenced this issue Jan 13, 2021
I overlooked Duration in #329. This adds the same options except that the
values for smallestUnit are preferred to be plural, and 'minutes' is not
allowed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
behavior Relating to behavior defined in the proposal documentation Additions to documentation non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants