Improve #inspect format for Time types #5794

straight-shoota · 2018-03-09T12:00:15Z

I'm doing some housekeeping to normalize and improve the inspect (and to_s) output of time and location related types.

Time#inspect now includes nanoseconds ~~unless it is 0~~. #to_s still omits them to avoid cluttering the output. When calling #inspect you typically expect a detailed output that allows to distinguish the difference between two instances. While nanoseconds are usually not relevant, it should still show up in case it is significant.

Time.now.inspect # => 2018-03-09 12:52:36.506736000 +01:00 Europe/Berlin
Time.now.to_s    # => 2018-03-09 12:52:36 +01:00 Europe/Berlin

Time::Zone now includes a #format method which is used to print the offset as +HH:mm:ss. This format is added to the #inspect output and also used by Time::Formatter#time_zone.

RX14 · 2018-03-09T12:09:08Z

I'd expect inspect to include nanoseconds even if nanoseconds was 0.

straight-shoota · 2018-03-09T13:03:25Z

IMO it just adds unnecessary clutter. And nanoseconds == 0 is pretty common because it is usually not included when serializing time data.

But I see the point, that it adds uncertainty when a time instance is printed, because it can mean nanoseconds is just omitted (e.g. when calling to_s) or actually 0.

What do you think about printing only one decimal place (.0) if it is 0?

RX14 · 2018-03-09T13:12:36Z

@straight-shoota yes that's fine

straight-shoota · 2018-03-09T15:27:47Z

(this answers a deleted comment)

ISO 8601 or RFC 3339 only support fixed offsets. Time instances cannot be entirely expressed in those formats, because they are essentially an ISO 8601 date with an attached location which provides zone transition rules.

Two instances of Time having different locations but sharing the same current offset should be distinguishable. They behave differently for example when adding a time span. Thus location is significant for inspecting a Time instance.

If time zone rules don't matter, you should just use UTC and/or fixed offset locations if necessary. Such instances can be expressed as ISO 8601 or RFC 3339 and the location is omitted in #inspect.

ysbaddaden · 2018-03-12T11:12:53Z

My review got lost, but in essence:

#inspect printing a dangling .0 is useless noise;
#to_s has no reason to skip nanoseconds (unless zero);
#to_s should have a location argument that defaults to Location::Local, allowing useful constructs such as Time.with(user.location) { puts comment.created_at.to_s }; bonus: we don't have to print the location (it's localtime or the specified one).

Actually, I believe all Time instances should be shifted to localtime —I'm probably wrong, but I never got a use-case where keeping the original location was essential or even useful. The fact that international standards (ISO 8601) don't specify locations is a huge hint to me that what's important is the offset (structural deviation from UTC), not the location (context specific representational concept).

straight-shoota · 2018-03-12T13:53:46Z

I now concur with @RX14 that nanoseconds should better be explicit. It's obviously very unlikely that this gets confused with a custom display of a non zero-nanosecond time (for example `time.to_s("%F %T %Z #{time.location}")), but I think we should better be safe than sorry. IMHO the additional noise is acceptable.

I'm not sure about to_s... could be with or without nanoseconds, I don't have a clear position if it should.

I like the idea of specifying a location for formatting, but I would just add it as an argument to #to_s:
comment.created_at.to_s(user.location). This would format the time in the given location without printing the actual location. It could probably even omit the offset because it is clearly a localized format.
It's essentially the same as comment.created_at.in(user.location).to_s but with a modified default format.

What do you mean with "all Time instances should be shifted to localtime"? Time#location should always be Location.local??

The location is usually not that important, that's true. But it depends on what you're doing with your Time instances. If it's just recording timed events and printing that in a localized format, then yes, you don't need the Time instance to have any location. It should just be UTC and it is only localized when printed.
But if you're doing calendar operations such as adding time spans, it is essential to have the Time instance in a specific location.

straight-shoota · 2018-03-12T22:44:33Z

After thinking about this, I'm pretty sure to_s should not print nanoseconds by default. Many Time instances will have a non-zero nanoseconds because they were created as Time.now but for the great majority of format use cases sub-second precision is completely irrelevant. Let's keep the default format short and precise for ~90% of use cases. It is always possible to specify a custom format for to_s.

Following this reasoning, I would rather consider removing the location from the default format than adding nanoseconds.

RX14 · 2018-03-13T00:26:36Z

nah, we should keep every detail including nanoseconds and zone on inspect and have to_s just be date time +-offset without ns.

straight-shoota · 2018-03-19T18:04:34Z

@RX14's suggestion seems reasonable, to_s gives the typically needed information and inspect shows everything. I've implemented it that way.

RX14 · 2018-03-19T23:38:16Z

needs rebase for CI fix

straight-shoota · 2018-03-26T13:32:33Z

Rebased

RX14

Docs seem to be out of date.

RX14 · 2018-03-26T14:48:43Z

src/time.cr

-        to_s "%F %T %:z", io
+  # Prints this `Time` to *io*.
+  #
+  # The local date-time is formatted as ISO 8601 date string `YYYY-MM-DD HH:mm:ss.nnnnnnnnn +ZZ:ZZ:ZZ`.


that string isn't iso 8601 and the example doesn't include the zone name.

Why isn't it ISO 8601?

Zone name is mentioned in the next paragraph.

no T, and there's a space between the nanoseconds and the +, and iso 8601 only permits hh:mm offset

T designator can be omitted, but the additional white space is true. The question is: should it be removed?

no, T can't be omitted in practice. We can talk about the standard details all we want but in practice iso 8601 means rfc3339. We should leave the format as-is but not advertise to_s as conforming to any spec.

RX14 · 2018-03-26T14:52:05Z

src/time.cr

    io
  end

+  # Prints this `Time` to *io*.
+  #
+  # The local date-time is formatted as ISO 8601 date string `YYYY-MM-DD HH:mm:ss +ZZ:ZZ:ZZ`.


also not iso 8601.

RX14 · 2018-03-26T14:52:38Z

src/time.cr

+  # The local date-time is formatted as ISO 8601 date string `YYYY-MM-DD HH:mm:ss +ZZ:ZZ:ZZ`.
+  # Nanoseconds are always omitted. Offset seconds are omitted if `0`.
+  #
+  # The name of the location is appended unless it is a fixed zone offset.


It shouldn't be.

straight-shoota · 2018-04-10T12:20:39Z

GTG?

`#inspect` now prints `nanoseconds` unless it is `0` and uses `Time::Zone#format`. Location name is no longer omitted if `"Local"`, otherwise there would be no distinction between a fixed offset time zone and local time zone because both would just show the offset and no name. `#to_s` still omits nanoseconds and time zone name.

straight-shoota · 2018-04-26T12:39:10Z

Merge and milestone?

* Improve #inspect format for Time::Location types * Add Time::Zone#format and use it in Time::Formatter * Improve format of Time#inspect `#inspect` now prints `nanoseconds` unless it is `0` and uses `Time::Zone#format`. Location name is no longer omitted if `"Local"`, otherwise there would be no distinction between a fixed offset time zone and local time zone because both would just show the offset and no name. `#to_s` still omits nanoseconds and time zone name.

straight-shoota force-pushed the jm/fix/time-to_s branch from d034325 to 5a77720 Compare March 9, 2018 13:04

straight-shoota force-pushed the jm/fix/time-to_s branch 3 times, most recently from 83a1f92 to 762a467 Compare March 9, 2018 14:09

straight-shoota force-pushed the jm/fix/time-to_s branch from 762a467 to e907c7a Compare March 9, 2018 20:51

straight-shoota force-pushed the jm/fix/time-to_s branch from e907c7a to b1169b3 Compare March 19, 2018 18:01

RX14 approved these changes Mar 19, 2018

View reviewed changes

straight-shoota added 2 commits March 26, 2018 13:20

Improve #inspect format for Time::Location types

eaabf3f

Add Time::Zone#format and use it in Time::Formatter

503461c

straight-shoota force-pushed the jm/fix/time-to_s branch from b1169b3 to f1cbddf Compare March 26, 2018 13:21

RX14 requested changes Mar 26, 2018

View reviewed changes

straight-shoota force-pushed the jm/fix/time-to_s branch from f1cbddf to f471ab1 Compare March 26, 2018 15:30

RX14 approved these changes Apr 11, 2018

View reviewed changes

sdogruyol approved these changes Apr 12, 2018

View reviewed changes

RX14 added topic:stdlib kind:refactor labels Apr 12, 2018

straight-shoota force-pushed the jm/fix/time-to_s branch from f471ab1 to 96bc5d3 Compare April 13, 2018 12:59

RX14 added this to the Next milestone Apr 26, 2018

RX14 merged commit 3afc781 into crystal-lang:master Apr 26, 2018

straight-shoota deleted the jm/fix/time-to_s branch April 26, 2018 13:21

straight-shoota mentioned this pull request Apr 26, 2018

Extend documentation of Time and Time::Location APIs #5819

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve #inspect format for Time types #5794

Improve #inspect format for Time types #5794

straight-shoota commented Mar 9, 2018 •

edited

RX14 commented Mar 9, 2018

straight-shoota commented Mar 9, 2018

RX14 commented Mar 9, 2018

straight-shoota commented Mar 9, 2018 •

edited

ysbaddaden commented Mar 12, 2018 •

edited

straight-shoota commented Mar 12, 2018

straight-shoota commented Mar 12, 2018 •

edited

RX14 commented Mar 13, 2018

straight-shoota commented Mar 19, 2018 •

edited

RX14 commented Mar 19, 2018

straight-shoota commented Mar 26, 2018

RX14 left a comment

RX14 Mar 26, 2018

straight-shoota Mar 26, 2018

straight-shoota Mar 26, 2018

RX14 Mar 26, 2018 •

edited

straight-shoota Mar 26, 2018

RX14 Mar 26, 2018 •

edited

RX14 Mar 26, 2018

RX14 Mar 26, 2018

straight-shoota commented Apr 10, 2018

straight-shoota commented Apr 26, 2018

Improve #inspect format for Time types #5794

Improve #inspect format for Time types #5794

Conversation

straight-shoota commented Mar 9, 2018 • edited

RX14 commented Mar 9, 2018

straight-shoota commented Mar 9, 2018

RX14 commented Mar 9, 2018

straight-shoota commented Mar 9, 2018 • edited

ysbaddaden commented Mar 12, 2018 • edited

straight-shoota commented Mar 12, 2018

straight-shoota commented Mar 12, 2018 • edited

RX14 commented Mar 13, 2018

straight-shoota commented Mar 19, 2018 • edited

RX14 commented Mar 19, 2018

straight-shoota commented Mar 26, 2018

RX14 left a comment

Choose a reason for hiding this comment

RX14 Mar 26, 2018

Choose a reason for hiding this comment

straight-shoota Mar 26, 2018

Choose a reason for hiding this comment

straight-shoota Mar 26, 2018

Choose a reason for hiding this comment

RX14 Mar 26, 2018 • edited

Choose a reason for hiding this comment

straight-shoota Mar 26, 2018

Choose a reason for hiding this comment

RX14 Mar 26, 2018 • edited

Choose a reason for hiding this comment

RX14 Mar 26, 2018

Choose a reason for hiding this comment

RX14 Mar 26, 2018

Choose a reason for hiding this comment

straight-shoota commented Apr 10, 2018

straight-shoota commented Apr 26, 2018

straight-shoota commented Mar 9, 2018 •

edited

straight-shoota commented Mar 9, 2018 •

edited

ysbaddaden commented Mar 12, 2018 •

edited

straight-shoota commented Mar 12, 2018 •

edited

straight-shoota commented Mar 19, 2018 •

edited

RX14 Mar 26, 2018 •

edited

RX14 Mar 26, 2018 •

edited