Improve #inspect format for Time types

[straight-shoota]

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]

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

[straight-shoota]

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]

@straight-shoota yes that's fine

[straight-shoota]

(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]

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]

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]

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]

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]

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

[RX14]

needs rebase for CI fix

[straight-shoota]

Rebased

[RX14]

Docs seem to be out of date.

[RX14]

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

[straight-shoota]

Why isn't it ISO 8601?

[straight-shoota]

Zone name is mentioned in the next paragraph.

[RX14]

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

[straight-shoota]

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

[RX14]

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]

also not iso 8601.

[RX14]

It shouldn't be.

[straight-shoota]

GTG?

[straight-shoota]

Merge and milestone?