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

Support different number of fraction digits for RFC3339 time format #9283

Merged
merged 5 commits into from
May 18, 2020

Conversation

waj
Copy link
Member

@waj waj commented May 12, 2020

This makes possible to select 0, 3, 6 or 9 decimal digits in RFC3339 time format.
Also I added the optional fraction_digits parameter to Time#to_rfc3339 and Time#to_rfc3339(IO)

Something I still feel weird is that by default (fraction_digits = nil) the output will print all the decimals or no decimals at all depending on the time value. I know it might be really unlikely that nanoseconds == 0 but still I think the output format should be always the same.
Maybe the default could be something in between, like printing microseconds?

@waj
Copy link
Member Author

waj commented May 13, 2020

I'd like to hear opinions about the default behaviour (fraction_digits = nil). I think the current is just not right and maybe just convenient to write the specs. Maybe it's better to use a fixed value, or dependent on the platform. Nanoseconds seems to be always 0 at least in my machine.

@straight-shoota
Copy link
Member

IIRC the reason for the default behaviour was that artifically created time values like Time.local(2020, 5, 13, 19, 54) rarely have sub-second components and displaying them as .000000000 is overspecific and suggests an higher accuracy than they actually have.
Instances obtained direclty from the time line (via Time.local or Time.utc) instead have (usually) non-zero sub-second components and they're typically relevant to tell closely situated instances apart.
Of course this is totally arbitrary. It tries to reflect different granularities.

Nanoseconds seems to be always 0 at least in my machine.

That seems very strange. A typical system clock should usually have microsecond resolution.

@waj
Copy link
Member Author

waj commented May 13, 2020

A typical system clock should usually have microsecond resolution

Yes, I meant I have microseconds, but no nanoseconds (2020-05-13T18:46:27.824647000Z)

@straight-shoota
Copy link
Member

Yeah, that's to be expected. Actual nanosecond resolution seems to be rare. But it doesn't matter in this regard, because even microsecond resolution makse nanosecond == 0 not true.

@waj
Copy link
Member Author

waj commented May 13, 2020

I know... what I'm saying is that it's plainly wrong to change the output format depending on the number of nanoseconds. We must choose a default. Could be nanoseconds, but if it's rare to have that level of precision, let's use microseconds. Or let's make it dependent on the platform.

@waj
Copy link
Member Author

waj commented May 13, 2020

I just went ahead and change the default to microseconds. We could make it platform dependent, but for portability might be better to have always the same value. Microseconds is probably enough for most applications anyway.

If we want the previous behaviour, we could add a precision field that stores up to how many digits of nanoseconds are reliable. This same approach is used by Elixir for example. Although I remember some headaches because a value with precision 0 is not equal to another one with higher precision but 0 microseconds.

Copy link
Member

@asterite asterite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Microseconds precision is good! 👍

@straight-shoota
Copy link
Member

I don't think microseconds by default is a good choice. It's worse than the status quo.
Microseconds look precise but can't express the full value range. Now different instances serialize identically if they only differ in the nanosecond scope. The previous behaviour made sure this could not happen.
The default should either be always nanoseconds or no sub-second component at all, or a combination of both (status quo) which is more acceptable than microseconds.
No sub-second component also leads to identical serialization of different instances but then it's obvious that the serialization is imprecise.

@jhass
Copy link
Member

jhass commented May 13, 2020

Is the disconnect maybe whether you see this as a format for machines to consume or for humans to read?

@waj
Copy link
Member Author

waj commented May 13, 2020

Actually in Ruby the default is to display no decimals. We could go that way too.

I don't think the current status is better. If you see a value without decimals you don't know if they were just not printed (due to formatting decision) or is actually zero nanoseconds. And the same happens with any number of decimals.

@asterite
Copy link
Member

I'm also thinking, if one days the computers have more precisions, should we go ahead and change the format to show more decimal digits? At which point you will be confident that no decimals were cut?

@straight-shoota
Copy link
Member

If you see a value without decimals you don't know if they were just not printed (due to formatting decision) or is actually zero nanoseconds.

That's true but it's obvious that this could be a deliberate choice. That's much less visible for microseconds.

@asterite I'm pretty sure we can safely assume nanoseconds to be the maximum precision used by general purpose computer systems for a long time.

@waj waj force-pushed the fix/rfc3339-fraction-digits branch from c899e21 to 4cc2b44 Compare May 14, 2020 15:40
@waj
Copy link
Member Author

waj commented May 14, 2020

I just pushed the change to make it work like in Ruby, with no decimals by default. For YAML, because it might be used as a serialization format, it's now always using 9 digits.

In the future we could add a :all option to write as many digits depending on the platform or the source of the value. For example, if I do Time.new(..., milliseconds: 123) then when formatting with :all only three digits would be printed. Just an idea to explore in a separate issue.

@waj waj added this to the 0.35.0 milestone May 14, 2020
@bcardiff bcardiff merged commit 5188e10 into crystal-lang:master May 18, 2020
@waj waj deleted the fix/rfc3339-fraction-digits branch June 8, 2020 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants