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

Rename Time#epoch to #to_unix #6662

Merged
merged 4 commits into from Oct 3, 2018

Conversation

Projects
None yet
7 participants
@straight-shoota
Contributor

straight-shoota commented Sep 4, 2018

Time.epoch and Time#epoch are somewhat misleading because they operate on seconds from Unix epoch (1970-01-01) but the epoch of Time is actually 0001-01-01.

A better name for Time#epoch is unix_seconds and fits nicely with #total_seconds (see #6661).
Similarly, #epoch_ms and #epoch_f are renamed to #unix_seconds_ms and #unix_seconds_f. Maybe the millisecond variant could also be #unix_ms or #unix_milliseconds.

Time#epoch is renamed to Time#to_unix. Similarly, #epoch_ms and #epoch_f are renamed to #to_unix_ms and #to_unix_f.

The supplementing constructors are renamed from Time.epoch and Time.epoch_ms to Time.unix and Time.unix_ms. A variant accepting a floating point number does not exist.

The following two commits just refactor some argument and variable names in Time::Location and Time::Format to use unix_seconds instead of epoch.

See #5346 (comment)

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/time-unix-epoch branch 2 times, most recently from 2d60706 to f78359e Sep 4, 2018

@ysbaddaden

This comment has been minimized.

Member

ysbaddaden commented Sep 5, 2018

Please, #unix_seconds is horrible. Probably more self-explanatory than #epoch but terrible nonetheless.

This is returning time under a new form, not accessing a property of time, so I believe it should be a #to_ method. For example #to_unix reads damn better.

I never see the need for the millisecond variant, and what about other precisions?

I wish we'd kept the .at, #to_i and #to_f methods of Ruby. It assumes that the number representation of time is an UNIX timestamp (in seconds), which was a simple convention. Especially since a "timestamp" always refers to an UNIX timestamp —nobody confuses them with windows epoch or whatever else.

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Sep 5, 2018

#to_unix sounds good.

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Sep 5, 2018

Unix timestamps with millisecond precision are commonly used in many time implementation. It has some practical importance as it is the highest resolution that can represent a substantial time range (> ±300 years) in only 64 bits. For example, it is used in YAML and JSON converters.

I'm not sure about the the relevance of floating point value, though. It can easily be calculated from #to_unix and #nanoseconds and isn't used anywhere in stdlib and usages in other shards available on Github shows it is mostly used incorrectly to measure elapsed time.

@asterite

This comment has been minimized.

Contributor

asterite commented Sep 10, 2018

I agree that epoch is not the best name for the methods. I like to_i and to_f from Ruby but if we want to support milliseconds and/or nanoseconds (for example Go has UnixNano() as a method) then to_i will be confusing. I think unix_seconds, unix_milliseconds and so on is a good choice.

There's an asymmetry in Time.unix and time.unix_seconds... maybe both should be named unix_seconds or just unix? I think it's common knowledge that "unix timestamp" refers to seconds, so we chould just leave it at unix and unix_ms for both instance and class methods. This again would be similar to Go (not a reason, just a remark).

@RX14

This comment has been minimized.

Member

RX14 commented Sep 10, 2018

to_i and to_f are very ambiguous on the epoch. I don't want to support them.

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Sep 10, 2018

I'm indecisive about unix vs. unix_seconds or @ysbaddaden's to_unix maybe this comparison helps:

unix unix_seconds Combination to_unix
constructor s Time.unix Time.unix_seconds Time.unix Time.unix
constructor ms Time.unix_ms Time.unix_milliseconds Time.unix_milliseconds / Time.unix_ms Time.unix_ms
seconds Time#unix Time#unix_seconds Time#unix_seconds Time#to_unix
milliseconds Time#unix_ms Time#unix_milliseconds Time#unix_milliseconds / Time.unix_ms Time#to_unix_ms
float (?) Time#unix_f Time#unix_seconds_f Time#unix_seconds_f Time#to_unix_f

I think I tend to prefer unix_seconds or a combination with unix for brevity.

I'm not sure Unix nanoseconds makes much sense when returning an Int64. It can only represent dates between 1678 and 2262. That probably includes 99% of use cases but it is not defined for dates outside that range. Considering that nanosecond precision is rarely really required I'd say that combining Unix seconds and #nanosecond should be plenty.

Maybe Unix timestamp with nanosecond precision could be added at some point when Int128 is fully supported.

@asterite

This comment has been minimized.

Contributor

asterite commented Sep 10, 2018

@straight-shoota I just mentioned nanoseconds as a name example in UnixNano, but I don't think we should add just methods for the reasons you mention above (and which are mentioned in the Go API).

Looking at the table, I'm starting to like the last column. The to_ prefix really shows that you are converting the time instance to something else, and Time.unix also looks good.

@j8r

This comment has been minimized.

Contributor

j8r commented Sep 10, 2018

Else a Time::Unix Object with all the stuff related to Unix, which will have also some methods of Time like #seconds, #milliseconds, #nanoseconds etc... A Time#to_unix can be used, as proposed above.

Assuming some_time is of the Time type: some_time.to_unix.seconds

@RX14

This comment has been minimized.

Member

RX14 commented Sep 10, 2018

I like the last column too.

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/time-unix-epoch branch 3 times, most recently from 3fafcb1 to ed4284c Sep 10, 2018

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Sep 10, 2018

Yeah, I guess to_unix makes sense and if nothing else, it's shorter than #unix_seconds :D

@j8r

This comment has been minimized.

Contributor

j8r commented Sep 10, 2018

Sorry, I got the idea to transform a time to an Unix epoch time, but it's forgetting what the Unix epoch time is really – the time elapsed since a date, conresponding to 1970-01-01 00:00:00 UTC.

Rust got it with the UNIX_EPOCH constant.
Example: SystemTime::now().duration_since(UNIX_EPOCH)

We don't have to create all this methods, we want to get time since a date, the one corresponding of the unix epoch.
For the ease of use, I agree with #to_unix.

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/time-unix-epoch branch from ed4284c to 2a60f9a Oct 2, 2018

@straight-shoota straight-shoota changed the title from Rename Time#epoch to #unix_seconds to Rename Time#epoch to #to_unix Oct 2, 2018

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Oct 2, 2018

Rebased and ready for review.

@j8r

This comment has been minimized.

Contributor

j8r commented Oct 3, 2018

I was thinking about something as simple as:

struct Time
  UNIX_EPOCH = Time.utc 1970, 1, 1

  def to_unix : Time::Span
    self - UNIX_EPOCH
  end

  def self.from_unix(unix_time : Time::Span) : Time                                   
    UNIX_EPOCH + unix_time                                                      
  end
end

Time.utc_now.to_unix.total_seconds

Edit: We could also have Time.from_unix

@RX14

This comment has been minimized.

Member

RX14 commented Oct 3, 2018

Apart from that tiny tiny nitpick, all good to go from me

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Oct 3, 2018

@j8r I like the idea to simplify the API avoiding several methods for different return types. That's definitely a plus.

Looking at the use cases in stdlib, the return value is usually used as an integer representing the number of seconds since Unix epoch. In these cases, to_unix would need to be changed to .to_unix.total_seconds. That's a little more verbose, but probably acceptable.

Getting the number of seconds since Unix epoch as integer is by far the most common use case and it is implemented as a simple integer subtraction. Putting a Time::Span in between adds a little bit more complexity. It includes some overhead in terms of additional calculations (boundary checks, nanoseconds etc.)
However, I can't confirm this using this benchmark. It's not very consistent, but span ranks about 5-10% faster than int on average. I'm not sure that's an accurate measurement, as it doesn't seem reasonable. Maybe there is too much optimization in the release build? (running the benchmark without --release, span is about 8 times slower than int. But that's not accurate either).

TIME = Time.utc_now

struct Time
  UNIX_EPOCH = Time.utc 1970, 1, 1

  def to_unix : Time::Span
    self - Time::UNIX_EPOCH
  end
end

Benchmark.ips do |bm|
  bm.report("int") do
    1000.times { TIME.epoch }
  end
  bm.report("span") do
    1000.times { TIME.to_unix.total_seconds }
  end
end

Maybe a compromise could be to have both, a method for directly getting the number of seconds as int and another method for getting a Time::Span which can be transformed into any unit you like. This could bring back my initial proposal #unix_seconds : Int64 along #to_unix : Time::Span.

@asterite

This comment has been minimized.

Contributor

asterite commented Oct 3, 2018

The only purpose of the time in unix is to get an integer that you can pass around to other systems, or store locally. I think getting a time span just makes the API difficult to use for no purpose.

straight-shoota added some commits Oct 3, 2018

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/time-unix-epoch branch from 2a60f9a to b129fd8 Oct 3, 2018

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Oct 3, 2018

The only purpose of the time in unix is to get an integer

Agreed. But there are currently different semantics supported: integer number of seconds, integer number of milliseconds, floating point number of seconds. They currently need three different methods on Time.
But it's really easy to get any semantic out of a Time::Span with #to_i, #total_miliseconds and #to_f, but also others such as #total_nanoseconds or #total_days.

Obviously, all other use cases besides integer number of seconds are less common and easy to implement in user code. So I tend to agree on keeping the PR as is, but returing a Time::Span could be a viable option.

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/time-unix-epoch branch from b129fd8 to 092c205 Oct 3, 2018

@j8r

This comment has been minimized.

Contributor

j8r commented Oct 3, 2018

If everyone strongly wants #unix_ methods...
In the meantime, this doesn't stop to have this methods and constant, or equivalent.
I personally think this 5 lines additions worth it, Time::Span offering nice possibilities.

@asterite

This comment has been minimized.

Contributor

asterite commented Oct 3, 2018

Yes, it's a lot of possibilities from which you'll always choose three in my opinion, so it's better to have those three methods, unless you have real use cases for this.

@j8r

This comment has been minimized.

Contributor

j8r commented Oct 3, 2018

  • #days is needed when interacting with /etc/shadow
  • #seconds is used in the gzip header, and more widely on UNIX
  • #milliseconds used in Crystal for JSON and YAML that use Time::EpochMillisConverter
  • #nanoseconds to have precise logs times, like done in Docker
@asterite

This comment has been minimized.

Contributor

asterite commented Oct 3, 2018

Right. That doesn't mean we should provide a more complex API for just one or two cases. Just my opinion though, but I don't remember any other API returning a unix time span instead of seconds/milliseconds/etc.

@j8r

This comment has been minimized.

Contributor

j8r commented Oct 3, 2018

Rust returns a SystemTime with .duration_since(SystemTime::UNIX_EPOCH)
Julia has various functions, including epochms2datetime(milliseconds) -> DateTime
Zig returns a TimeStamp with unixEpoch()

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Oct 3, 2018

@j8r In #6662 (comment), you probably meant to prefix every property with total_, right? (except #to_i instead of #seconds)

But #total_days is really a very rare unit for Unix time stamps.

@asterite

This comment has been minimized.

Contributor

asterite commented Oct 3, 2018

Disregard all my comments.

@RX14

This comment has been minimized.

Member

RX14 commented Oct 3, 2018

julia and zig are counterexamples, the only language which returns a datatype representing a duration, not an instant, is rust

@bcardiff

I think the history can be squashed for this PR.
I like the new methods, let's hope the time api is coming to a solid point for the users soon.

@RX14 RX14 added this to the 0.27.0 milestone Oct 3, 2018

@RX14 RX14 merged commit 4e1fc11 into crystal-lang:master Oct 3, 2018

4 checks passed

ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@RX14 RX14 added the kind:refactor label Oct 3, 2018

@straight-shoota straight-shoota deleted the straight-shoota:jm/feature/time-unix-epoch branch Oct 3, 2018

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Oct 3, 2018

@RX14 When a number datatype is returned, it also represents a duration, the number of seconds since Unix epoch. It's just debatable if the duration should be expressed as Time::Stamp which is Crystal's default data type for durations, or as primitive number types. This debate has been resolved, it seems 😄

let's hope the time api is coming to a solid point for the users soon.

I think we're getting there! 💯

@andrewrk

This comment has been minimized.

andrewrk commented Oct 3, 2018

Zig returns a TimeStamp with unixEpoch()

Hi everybody I just want to point out that the above link is to the language reference explaining how to use Documentation Comments (which you can see from the URL) and in no way describes what is available in the Zig Standard Library. In fact, there is no such TimeStamp structure, it does not exist!

I have not yet personally audited the time API of Zig, which can be found here: https://github.com/ziglang/zig/blob/master/std/os/time.zig.

@RX14

This comment has been minimized.

Member

RX14 commented Oct 3, 2018

(@andrewrk how did you even find this issue?!?!)

@andrewrk

This comment has been minimized.

andrewrk commented Oct 3, 2018

#!/bin/sh
  
set -e

s3cmd sync s3://andrewrk/log/ziglang.org/ ziglang.org/
s3cmd del s3://andrewrk/log/ziglang.org/ --recursive
echo -n | gzip >ziglang.org/empty.gz
cat ziglang.org/* | gunzip >>all.txt
rm ziglang.org/*
cat all.txt | cut "--delimiter= " -f10 | sort | uniq >new.txt
touch prev.txt
diff -u prev.txt new.txt | cdiff
mv new.txt prev.txt

somebody clicked the link

arcage added a commit to arcage/crystal that referenced this pull request Nov 5, 2018

Fix `#unix_ms` to `#to_unix_ms` in CANGELOG.md
The CHANGELOG for 0.27.0 shows that some instance method names are changed in Time class.

> - **(breaking-change)** Rename `Time#epoch` to `Time#to_unix`. Also `#epoch_ms` to `#to_unix_ms`, and `#epoch_f` to `#to_unix_f`. ([crystal-lang#6662](crystal-lang#6662), thanks @straight-shoota)

Actually, `#epoch_ms` was renamed to `#to_unix_ms`, not `#unix_ms`.

RX14 added a commit that referenced this pull request Nov 7, 2018

Fix `#unix_ms` to `#to_unix_ms` in CHANGELOG.md (#7024)
The CHANGELOG for 0.27.0 shows that some instance method names are changed in Time class.

> - **(breaking-change)** Rename `Time#epoch` to `Time#to_unix`. Also `#epoch_ms` to `#to_unix_ms`, and `#epoch_f` to `#to_unix_f`. ([#6662](#6662), thanks @straight-shoota)

Actually, `#epoch_ms` was renamed to `#to_unix_ms`, not `#unix_ms`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment