Change Time constructor to have `nanosecond` & `kind` as named argument #5072

bew · 2017-10-03T13:24:08Z

I think this should address #5069 (comment)

Somehow I can't get the spec to pass, I always get:

Error in spec/std/time/time_spec.cr:25: no argument named 'millisecond'
Matches are:
 - Time.new(year, month, day, hour = 0, minute = 0, second = 0, *, nanosecond = 0, kind = Kind::Unspecified) (trying this one)
 - Time.new(time : LibC::Timespec, kind = Kind::Unspecified)
 - Time.new()
 - Time.new(*, seconds : Int64, nanoseconds : Int32, kind : Kind)

    t3 = Time.new 2002, 2, 25, 15, 25, 13, millisecond: 8
              ^~~

As if it doesn't find the other constructor with millisecond, and I don't see what I'm missing..

In addition to have millisecond & nanosecond, there is also kind as required named argument, I think it is a safe breaking change.

RX14 · 2017-10-03T13:25:26Z

src/time.cr

+      raise ArgumentError.new "Invalid time"
+    end
+
+    new(year, month, day, hour, minute, second, nanosecond: millisecond * 1_000, kind: kind)


You should use the NANOSECONDS_PER_MILLISECOND constant. It even has the right number.

RX14 · 2017-10-03T13:35:17Z

def test(*, foo = 0)
  puts "foo"
end
  
def test(*, bar = 0)
  puts "bar"
end

test(bar: 5)
test(foo: 5)

Error in line 10: no argument named 'foo'
Matches are:
 - test(*, bar = 0)

Surely this is a bug, and the second method shouldn't overload the first, because their arguments are named differently and the calls are entirely unambiguous.

EDIT: removing the = 0 fixes it, it's a bug with normalizing default arguments then, I guess?

asterite · 2017-10-03T13:37:49Z

CI failed. Please make sure to run the specs locally (I think running bin/crystal spec/std_spec.cr should be enough). I was pretty sure that Time constructor was being used in more places.

bew · 2017-10-03T13:39:09Z

@asterite I know they fail, I've mentioned it in the description, see the last message from @RX14 as to why!

asterite · 2017-10-03T13:49:33Z

Oh, a bug. So maybe just remove the millisecond constructor for now and leave the nanosecond one.

asterite · 2017-10-03T13:50:45Z

It's actually not a bug, because if you invoke it without arguments then there's no way to choose one or the other. So the default value can't be present in both overloads. Let's either remove the millisecond constructor or just require millisecond and nanosecond without default values (I think I prefer the first option).

Papierkorb · 2017-10-03T13:56:49Z

@asterite, as there is a constructor taking no arguments, doesn't that resolve the ambiguity? The compiler sees a call to new(), sees three possible choices new(), new(*, milliseconds) and new(*, nanoseconds).

The only possible match is new(), as neither of the required arguments milliseconds nor nanoseconds were given, hence, these can't be it.

The issue only arises if you'd have new() and new(*, milliseconds = 0).

Or am I overlooking something here?

akzhan · 2017-10-03T13:58:08Z

A lot of PRs related to Time. I think that we should forget about milliseconds in constructor, just use parser with %L specifier.

bew · 2017-10-03T14:00:33Z

Or maybe instead of having:
def self.new(year, month, day, hour = 0, minute = 0, second = 0, *, millisecond = 0, kind = Kind::Unspecified)
def self.new(year, month, day, hour = 0, minute = 0, second = 0, *, nanosecond = 0, kind = Kind::Unspecified)

We should have:

def self.new(year, month, day, hour = 0, minute = 0, second = 0, *, kind = Kind::Unspecified)
def self.new(year, month, day, hour = 0, minute = 0, second = 0, *, millisecond, kind = Kind::Unspecified)
def self.new(year, month, day, hour = 0, minute = 0, second = 0, *, nanosecond, kind = Kind::Unspecified)

So that when one specify the constructor without precision, it'll work, and when using the millisecond or nanosecond ctor, they'll always have to specify a number. WDYT?

(after re-reading your comment @asterite, that's your second option)

asterite · 2017-10-03T14:08:45Z

I personally don't care about breaking code because I'm pretty sure no one uses that long Time constructor. So just having nanosecond there is fine. Maybe like what I originally did. So many discussions for something we think might break but we have no clue...

I'm a bit tired of these discussions right now. I think #5069 and #5070 should be merged, and this must be closed.

bcardiff

I agree with having strict named arguments for all second subdivisions overloads.
And default overload that would be up to second precision.

bcardiff · 2017-10-03T15:08:09Z

src/time.cr

@@ -140,7 +140,15 @@ struct Time
    new(seconds: seconds + offset, nanoseconds: nanoseconds, kind: Kind::Local)
  end

-  def self.new(year, month, day, hour = 0, minute = 0, second = 0, nanosecond = 0, kind = Kind::Unspecified)
+  def self.new(year, month, day, hour = 0, minute = 0, second = 0, *, millisecond = 0, kind = Kind::Unspecified)
+    unless 0 <= millisecond <= 999_999


shouldn't milliseconds should be between 0 and 999.
Otherwise we are speaking of microseconds. Which, could be other overload if needed.

Oh right, I knew I was missing something here... thanks!

RX14 · 2017-10-03T15:11:05Z

Perhaps it doesn't break anything, but I like this design more anyway. If the milliseconds overload is non-obvious, which I think it is, it's much better to name it something for moire readable code. I don't mind not having the milliseconds overload, but requiring nanoseconds to be named just feels like good design.

akzhan · 2017-10-03T15:14:50Z

Idea with named arguments will be better with microseconds accessor too.

asterite · 2017-10-03T15:20:39Z

I left the milliseconds params and accessor just for backwards compatibility, but we can safely remove them. I think working with nanoseconds everywhere is better.

RX14 · 2017-10-03T15:21:40Z

We simply have to remove = 0 from the milliseconds argument of the new constructor. There should be no ambiguity after that.

RX14 · 2017-10-03T15:22:43Z

src/time.cr

@@ -140,7 +140,15 @@ struct Time
    new(seconds: seconds + offset, nanoseconds: nanoseconds, kind: Kind::Local)
  end

-  def self.new(year, month, day, hour = 0, minute = 0, second = 0, nanosecond = 0, kind = Kind::Unspecified)
+  def self.new(year, month, day, hour = 0, minute = 0, second = 0, *, millisecond = 0, kind = Kind::Unspecified)


This constructor should never be called if millisecond isn't specified (no conversion required) so simply remove the default value and specs should pass.

asterite · 2017-10-03T15:34:02Z

But why do we want to keep that constructor?

In any case, I doubt that Time constructor is being used outside the std and specs, so it's better to just have the nanoseconds constructor.

bew · 2017-10-03T15:34:40Z

@bcardiff please review the numbers in my last commit 😄 (specs are passing for me, I removed the default value for the millisecond ctor).

RX14 · 2017-10-03T17:11:08Z

Another option is to remove the nanoseconds or milliseconds argument entirely, and simply use + 100.milliseconds or similar when required. I think this pr is fine though.

RX14 · 2017-10-03T17:17:26Z

Please rebase this on master.

bcardiff · 2017-10-03T17:24:29Z

spec/std/time/time_spec.cr

+
+  it "initialize milliseconds 1_000_000" do
+    Time.expect_invalid do
+      Time.new(9999, 12, 31, 23, 59, 59, millisecond: 1_000_000)


This spec should be with millisecond: 1_000

bcardiff · 2017-10-03T17:28:54Z

@RX14 although I defend + 1.days and similar constructs, I won't use 1.milliseconds unless they return proper timespans also. And it seems a bit overkilling to use timespans and a time to build another time instance.

bew · 2017-10-03T17:37:22Z

Rebased & squashed!

bew · 2017-10-03T17:41:53Z

spec/std/time/time_spec.cr

+  end
+
+  it "initialize max with milliseconds" do
+    time = Time.new(9999, 12, 31, 23, 59, 59, millisecond: 999)


Is this correct? or should we use 999.999_999 so that it expands to 999_999_999 nanoseconds?

999.999_999 won't pass the 0 <= millisecond <= 999
I think the issue is the name of the spec.

we could then use 0 <= millisecond < 1_000 ?

I think they are restricted to be integer since the initialize(*, @seconds : Int64, @nanoseconds : Int32, @kind : Kind) is called at the end of the day

We could add .to_i just before calling the Int32 restricted constructor. I think that would work OK.

I prefer them integer actually. All of them. But that's just my opinion 🤷‍♂️

ysbaddaden · 2017-10-03T19:02:00Z

I support @asterite, there is no need for many constructors and forcing kwargs on this.

Why reintroduce a millisecond constructor?

Because we used to have this precision? We musn't care about legacy in pre 1.0.
Because it's a nice addition? Sure, why not, but what about microseconds? Shall we introduce yet another constructor? Or shall we not, and have an arbitrary choice of milli/nano but not micro?

RX14 · 2017-10-03T21:20:54Z

I agree that perhaps having both constructors is perhaps too much, but I think we should keep the kwargs requirement even if we loose the milliseconds constructor (which appears to be the consensus).

straight-shoota · 2017-10-04T08:38:23Z

I don't think there should be a constructor accepting milliseconds. It's totally fine to just use nanoseconds, makes everything simpler to use just one grade of precision.
And it's probably fine to have nanoseconds as a named-only argument. Though I don't really care about that part because it's not a common use case outside of specs.

bew · 2017-10-04T09:40:47Z

src/yaml/schema/core/time_parser.cr

@@ -109,7 +109,7 @@ struct YAML::Schema::Core::TimeParser

    return nil if @reader.has_next?

-    time = new_time(year, month, day, hour, minute, second, millisecond: millisecond)
+    time = new_time(year, month, day, hour, minute, second, nanosecond: millisecond * 1_000_000)


I doesn't feel to be the good way to get the nanoseconds out of milliseconds.
Should we allow a Time::Span to be passed, so we would write: (..., nanosecond: millisecond.nanoseconds) ?

Why should this not be a good way? Multiplying by 1_000_000 is the proper transformation from milli do nano.

probably, maybe I just don't like to see some numeric constants written in code like that, but it's totally fine actually.

bew · 2017-10-04T15:55:15Z

@bcardiff @asterite @ysbaddaden another review?

bcardiff

A bit more of time is needed at some places. 999999ns actually. 🙃

bcardiff · 2017-10-04T17:05:29Z

src/time.cr

@@ -500,7 +500,7 @@ struct Time
    end
  end

-  def_at(end_of_year) { Time.new(year, 12, 31, 23, 59, 59, 999, kind: kind) }
+  def_at(end_of_year) { Time.new(year, 12, 31, 23, 59, 59, nanosecond: 999, kind: kind) }


these at_end_of_* are not returning the nearest representable time before the next interval.
nanosecond: 999_999_999 should probably be used instead of the old 999 milliseconds value.

I am not sure how this is playing when printing number with millisecond precision. It should truncate and not round to.

You're totally right, thank a lot for your in-depth review! I fixed it 😄

bcardiff · 2017-10-04T17:05:57Z

src/time.cr

@@ -509,7 +509,7 @@ struct Time
    else
      month, day = 12, 31
    end
-    Time.new(year, month, day, 23, 59, 59, 999, kind: kind)
+    Time.new(year, month, day, 23, 59, 59, nanosecond: 999, kind: kind)


bcardiff · 2017-10-04T17:06:29Z

src/time.cr

@@ -523,10 +523,10 @@ struct Time
    else
      month, day = 12, 31
    end
-    Time.new(year, month, day, 23, 59, 59, 999, kind: kind)
+    Time.new(year, month, day, 23, 59, 59, nanosecond: 999, kind: kind)


bcardiff · 2017-10-04T17:06:34Z

src/time.cr

  end

-  def_at(end_of_month) { Time.new(year, month, Time.days_in_month(year, month), 23, 59, 59, 999, kind: kind) }
+  def_at(end_of_month) { Time.new(year, month, Time.days_in_month(year, month), 23, 59, 59, nanosecond: 999, kind: kind) }


bcardiff · 2017-10-04T17:07:08Z

src/time.cr

-  def_at(midday) { Time.new(year, month, day, 12, 0, 0, 0, kind: kind) }
+  def_at(end_of_day) { Time.new(year, month, day, 23, 59, 59, nanosecond: 999, kind: kind) }
+  def_at(end_of_hour) { Time.new(year, month, day, hour, 59, 59, nanosecond: 999, kind: kind) }
+  def_at(end_of_minute) { Time.new(year, month, day, hour, minute, 59, nanosecond: 999, kind: kind) }


bew · 2017-10-04T18:42:51Z

spec/std/time/time_spec.cr

@@ -34,15 +34,15 @@ describe Time do
    time.nanosecond.should eq(999_999_999)
  end

-  it "initialize millisecond negative" do
+  it "initialize nanosecond negative" do


I think this description should be fail initialize with negative nanosecond WDYT?

Sure. There are many specs in std with very crude labels... ;)

bew · 2017-10-04T18:43:46Z

spec/std/time/time_spec.cr

    Time.expect_invalid do
-      Time.new(9999, 12, 31, 23, 59, 59, -1)
+      Time.new(9999, 12, 31, 23, 59, 59, nanosecond: -1)
    end
  end

  it "initialize nanoseconds 1_000_000_000" do


ditto, with fail initialize when nanosecond is a second or something like that

straight-shoota · 2017-10-04T19:40:33Z

@bcardiff in line comment

I am not sure how this is playing when printing number with millisecond precision. It should truncate and not round to.

This looks like a serious yet interesting issue, though I believe it is to much for this PR...

bew · 2017-10-04T21:29:55Z

@bcardiff I fixed the missing times in 999999ns!
I also went through the changes to check for logic or mistakes (like you probably did on the last review), I think it's OK now!

I'll squash the changes with proper commit message on your approval

bcardiff

LGTM. Sorry @bew for the many small reviews. Thanks for all the fixes!

bew · 2017-10-05T07:56:57Z

No problem @bcardiff, it keeps the PR alive 😄 (and fix things!!)

I've squashed the commits & rebased, so it should be ready to merge!

ysbaddaden

I'm not fond of forcing kwargs; it looks nice with literals (and it's encouraged) such nanosecond: 123, but leads to noisy repetitions with variables, such as nanosecond: nanosecond, kind: kind, as if the constructor wasn't large enough already.

I wish we'd only force kwargs for a couple releases, to help catch potential bugs (most likely none), then remove it and break nothing, because crystal is so cool :)

bcardiff · 2017-10-05T13:30:56Z

@ysbaddaden in this case the kwarg make it clearer since the subdivision is nos obvious from the argument. Plus due to the change of time precision it would help catch potential silent bugs.

I like also the artifact of doing slight breaking changes across versions to allow safe migration from one to another. Similar to what I propose for IO refactoring into classes.

But in this case I would stick with the kwarg, otherwise is to ambiguos IMO.

bcardiff · 2017-11-22T01:16:45Z

@bew I just noticed the Time::Span use nanoseconds as argument name (in plural), but Time use nanosecond (in singular).

As a non-native english speaker, is there a problem if both are unified in the plural form? That would match both types and also de ivar of Time.

ysbaddaden · 2017-11-22T09:48:45Z

I believe the difference is that one tells the nanosecond of a time, whereas in the other this is the number of nanoseconds in a time span.

bew · 2017-11-22T10:08:22Z

@bcardiff When first reading your comment, I agreed that it would be coherent to have nanoseconds everywhere.
But looking at the actual code, I'm not sure:

In the Time constructor with nanosecond, all arguments are singular:

def self.new(year, month, day, hour, minute, second, *, nanosecond, kind)

In the same way, (almost) all Time getter are singular: year, month, day, hour, minute, second, millisecond, nanosecond.

Should those getters be plural? I don't think so.
Should that constructor arguments be plural? Maybe.

ysbaddaden · 2017-11-22T11:16:23Z

@bew see my comment above. Time getters are singular because it's a snapshot of a time (singular), it's not elapsed seconds (plural). Time constructors must follow otherwise it would be inconsistent, and confusing, since it could mean "give me time at years + months + ... + seconds + nanoseconds".

RX14 reviewed Oct 3, 2017

View reviewed changes

bew force-pushed the time-new-milli-nano branch 2 times, most recently from ac0dca7 to 4bcb859 Compare October 3, 2017 13:28

bcardiff requested changes Oct 3, 2017

View reviewed changes

RX14 reviewed Oct 3, 2017

View reviewed changes

bew force-pushed the time-new-milli-nano branch 4 times, most recently from d6dafea to c7cd6af Compare October 3, 2017 16:29

bcardiff requested changes Oct 3, 2017

View reviewed changes

bew force-pushed the time-new-milli-nano branch from 2d30867 to 29cf460 Compare October 3, 2017 17:36

bew commented Oct 3, 2017

View reviewed changes

bew changed the title ~~Add constructor with millisecond & nanosecond as named argument~~ Change Time constructor to have nanosecond as named argument Oct 4, 2017

bew commented Oct 4, 2017

View reviewed changes

RX14 approved these changes Oct 4, 2017

View reviewed changes

bew changed the title ~~Change Time constructor to have nanosecond as named argument~~ Change Time constructor to have nanosecond & kind as named argument Oct 4, 2017

bcardiff requested changes Oct 4, 2017

View reviewed changes

bcardiff mentioned this pull request Oct 4, 2017

YAML/Time parsers/formatters now take in care nanoseconds #5070

Merged

bew commented Oct 4, 2017

View reviewed changes

Change Time constructor to have nanosecond & kind as named argument

1198566

bcardiff approved these changes Oct 4, 2017

View reviewed changes

bew force-pushed the time-new-milli-nano branch from 901b249 to 1198566 Compare October 5, 2017 07:55

straight-shoota approved these changes Oct 5, 2017

View reviewed changes

ysbaddaden reviewed Oct 5, 2017

View reviewed changes

bcardiff merged commit bd55733 into crystal-lang:master Oct 5, 2017

bcardiff added breaking-change topic:stdlib labels Oct 5, 2017

bcardiff added this to the Next milestone Oct 5, 2017

bew deleted the time-new-milli-nano branch October 5, 2017 14:09

Change Time constructor to have nanosecond & kind as named argument #5072

Change Time constructor to have nanosecond & kind as named argument #5072

Conversation

bew commented Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 commented Oct 3, 2017 • edited Loading

asterite commented Oct 3, 2017

bew commented Oct 3, 2017

asterite commented Oct 3, 2017

asterite commented Oct 3, 2017 • edited Loading

Papierkorb commented Oct 3, 2017

akzhan commented Oct 3, 2017

bew commented Oct 3, 2017 • edited Loading

asterite commented Oct 3, 2017 • edited Loading

bcardiff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 commented Oct 3, 2017

akzhan commented Oct 3, 2017

asterite commented Oct 3, 2017

RX14 commented Oct 3, 2017

Choose a reason for hiding this comment

asterite commented Oct 3, 2017

bew commented Oct 3, 2017 • edited Loading

RX14 commented Oct 3, 2017

RX14 commented Oct 3, 2017

Choose a reason for hiding this comment

bcardiff commented Oct 3, 2017

bew commented Oct 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bew Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ysbaddaden commented Oct 3, 2017

RX14 commented Oct 3, 2017

straight-shoota commented Oct 4, 2017 • edited Loading

bew Oct 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bew Oct 4, 2017 • edited Loading

Choose a reason for hiding this comment

bew commented Oct 4, 2017 • edited Loading

bcardiff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bew Oct 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

straight-shoota commented Oct 4, 2017

bew commented Oct 4, 2017 • edited Loading

bcardiff left a comment

Choose a reason for hiding this comment

bew commented Oct 5, 2017 • edited Loading

ysbaddaden left a comment

Choose a reason for hiding this comment

bcardiff commented Oct 5, 2017

bcardiff commented Nov 22, 2017 • edited Loading

ysbaddaden commented Nov 22, 2017

bew commented Nov 22, 2017 • edited Loading

ysbaddaden commented Nov 22, 2017

Change Time constructor to have `nanosecond` & `kind` as named argument #5072

Change Time constructor to have `nanosecond` & `kind` as named argument #5072

bew commented Oct 3, 2017 •

edited

Loading

RX14 commented Oct 3, 2017 •

edited

Loading

asterite commented Oct 3, 2017 •

edited

Loading

bew commented Oct 3, 2017 •

edited

Loading

asterite commented Oct 3, 2017 •

edited

Loading

bew commented Oct 3, 2017 •

edited

Loading

bew Oct 3, 2017 •

edited

Loading

straight-shoota commented Oct 4, 2017 •

edited

Loading

bew Oct 4, 2017 •

edited

Loading

bew Oct 4, 2017 •

edited

Loading

bew commented Oct 4, 2017 •

edited

Loading

bew Oct 4, 2017 •

edited

Loading

bew commented Oct 4, 2017 •

edited

Loading

bew commented Oct 5, 2017 •

edited

Loading

bcardiff commented Nov 22, 2017 •

edited

Loading

bew commented Nov 22, 2017 •

edited

Loading