Add documentation and type annotations for Time #5093

oprypin · 2017-10-09T17:18:16Z

No description provided.

bew · 2017-10-09T17:51:37Z

src/time.cr

@@ -181,7 +181,7 @@ struct Time
  # ```
  # Time.epoch(981173106) # => 2001-02-03 04:05:06 UTC
  # ```
-  def self.epoch(seconds : Int) : self
+  def self.epoch(seconds : Int) : Time


Why not keep self here?

If something subclasses Time, this will not hold anymore (uhh subclassing a struct is impossible, but the theoretic point still stands)

It can be confused with actually returning self (but modified). This way it's clear that it's an entirely new instance.

hmm I'm not sure I understand, do you think that if one subclasses Time to Time2 (in theory), the returned type of Time.epoch could be understood as Time2 ? Is that what you're trying to avoid? (TIL something! I thought it was just a shortcut to the current class)

Well, it is a class-level method, so it won't be able to 'return' self in any case.

Also, I think the doc generator should change self to the actual type (when it's used as a type) when possible & known.

Actually, since epoch calls new, which can be overloaded by the theoretical subclass (if s/struct/class/), it could return anything!

It's theoretical, Time is fine.

RX14 · 2017-10-09T23:17:23Z

src/time.cr

    self
  end

-  def +(other : Span)
+  # Returns a `Time` that is later by this `Time::Span`.


s/this/a/? hmm.

"later by this timespan" doesn't quite sound right to me.

I mean... I had already considered at least 4 options and none sound right to me. What do?

C# uses "Returns a new DateTime that adds the value of the specified TimeSpan to the value of this instance." I don't like that though.

So how about "Returns a new Time which is later than this Time by the Time::Span span", and rename the parameter to span.

RX14 · 2017-10-11T14:56:00Z

src/time.cr

@@ -203,12 +203,12 @@ struct Time
    self
  end

-  # Returns a `Time` that is later by this `Time::Span`.
+  # Returns the `Time` that is later by this amount.


I prefer a to the here (and everywhere else).

OK, reverted that

oprypin · 2017-10-12T22:02:32Z

Please merge? 😐
@ysbaddaden, what do you think?

RX14 · 2017-10-13T09:04:10Z

src/time.cr

@@ -275,76 +281,90 @@ struct Time
    end
  end

-  def self.now : self
+  # Returns the current time in the current time zone.


I think "local time zone" is a better wording here.

RX14 · 2017-10-13T09:04:45Z

src/time.cr

    Time.new(year, month, day, kind: kind)
  end

-  def year
+  # Returns the year number (Common Era).


"Returns the year number (in the Common Era)"?

RX14 · 2017-10-13T09:05:56Z

src/time.cr

    year_month_day_day_year[1]
  end

-  def day
+  # Returns the day number of the month (`0..31`).


Surely it's 1..31 to fit in with month?

RX14 · 2017-10-13T09:09:41Z

src/time.cr

    year_month_day_day_year[2]
  end

-  def hour
+  # Returns the hour of the day (`0...24`).


Why not 0..23? I think it's clearer. What about leap seconds? We should document that as an exception in addition to the "standard range" 0..23.

The day is defined as 24 hours and it's also clearer that time goes up to almost 24, not just 23. I will not be making this change.
I also don't know about leap seconds, and whether they are even possible in this implementation.

A leap second doesn't lead to 24:00:00. It is represented as 23:59:60. So the only field that might have a irregular maximum value because of leap seconds is second.

And I'd strongly suggest to use 0..23 because this accurately represents the value range of hour. There is no "almost 24", it is not represented in this granularity. Times from 23:00:00 to 23:59:59 (or :60) are all hour 23 without any further distinction. There is no sense in using an exclusive range here.
Besides, the exact meaning of the exclusive range literal ... is not as commonly known and easily understood as the inclusive range ..

I'm even more convinced this is bad now.

RX14 · 2017-10-13T09:15:43Z

src/time.cr

    (total_seconds % SECONDS_PER_MINUTE).to_i
  end

-  def millisecond
+  # Returns the millisecond of the second (`0...1000`).


Ditto on 0..999.

Ditto. 😛

RX14 · 2017-10-13T09:54:08Z

src/time/span.cr

    (to_i.to_f * NANOSECONDS_PER_SECOND) + nanoseconds
  end

-  def to_f
-    total_seconds
+  # Converts to a number of milliseconds.


This can be fractional too, right?

Yes but come on, it's to_f

RX14 · 2017-10-13T09:55:06Z

src/time/span.cr

    abs
  end

-  def abs
+  # Returns a non-negative `Time::Span` by removing the sign from this one.


"Returns the absolute, non-negative amount of time this Time::Span represents by removing the sign."

RX14 · 2017-10-13T09:56:14Z

src/time/span.cr

    Span.new(seconds: to_i.abs, nanoseconds: nanoseconds.abs)
  end

-  def from_now
+  # Returns a `Time` that happens later by `self` than the current time.


"Returns a Time which is in the future by this Time::Span.

I disagree with this. The method name already conveys what you're saying, so this description rephrases it in a different, more explicit way.

Another way is "Returns a Time that results of adding span to this time"

@asterite "... to the current time." But otherwise I'd vote for this suggestion.

RX14 · 2017-10-13T09:56:20Z

src/time/span.cr

    Time.now + self
  end

-  def ago
+  # Returns a `Time` that happens earlier by `self` than the current time.


RX14 · 2017-10-13T10:22:55Z

src/time/span.cr

    to_i == 0 && nanoseconds == 0
  end
 end

 struct Int
-  def week
+  # :nodoc:


That was exactly what I thought when I saw this abomination.
"We don't want aliases, oh except for when they make a kewl DSL even cooler."
If someone has been conditioned to like this, they can keep using it, but new people don't have to know this.

It's a big amount of noise in the docs, so I did not want to document it. But I also didn't want to not document it.

If you all agree on removing these aliases it's fine with me. Maybe it's not that bad to write 1.minutes instead of 1.minute.

Either have them and document it, or not have them. I'd vote for the latter.

asterite · 2017-10-13T20:37:29Z

src/time.cr

@@ -452,7 +474,8 @@ struct Time
    epoch.to_f + nanosecond.to_f / 1e9
  end

-  def to_utc
+  # Converts this time to UTC.


Maybe "converts" can be understood as if it mutates the instance, maybe something like "Returns a new Time instance where ..." but I don't know how to reword it.

asterite · 2017-10-13T20:39:51Z

src/time/span.cr

  end

-  def duration
+  # Alias of `abs`.
+  def duration : Time::Span


We can probably remove this alias in a later PR.

bew · 2017-10-13T20:47:21Z

src/time.cr

@@ -474,7 +474,7 @@ struct Time
    epoch.to_f + nanosecond.to_f / 1e9
  end

-  # Converts this time to UTC.
+  # Returns a copy of this `Time` converted to UTC.


Not sure it's important, but it's not always a copy: if self is already an UTC time, it returns self, not a copy of it.

As a struct, it's still a copy.

@oprypin Thanks!

oh right I forgot that minor fact

RX14 · 2017-10-13T21:09:22Z

For immutable objects, copy and not are indistinguishable. Especially for structs.

straight-shoota

Looks really good, great work! However there are a few suggestions for further improving the wording.

straight-shoota · 2017-10-14T11:45:53Z

src/time.cr

    self
  end

-  def +(other : Span)
-    add_span other.to_i, other.nanoseconds
+  # Returns a `Time` that is later than this `Time` by the `Time::Span` *span*.


"Returns a Time that is later than this by adding span."

Adding time is not a thing. That's why I explain "+" (that already conveys the "adding" part) with a real-world concept. So I think this suggestion is not an improvement. Same for the other ones.

I think it is, that's what this method is for. The main part of my suggestion was anyway to remove the imo unnecessary types.

straight-shoota · 2017-10-14T11:46:12Z

src/time.cr

  end

-  def -(other : Span)
-    add_span -other.to_i, -other.nanoseconds
+  # Returns a `Time` that is earlier than this `Time` by the `Time::Span` *span*.


"Returns a Time that is earlier than this by subtracting span."

straight-shoota · 2017-10-14T11:47:13Z

src/time.cr

  end

-  def +(other : MonthSpan)
+  # Adds a number of months specified by *other*'s value.
+  def +(other : Time::MonthSpan) : Time


other => span (or month_span)

And doc: "Returns a Time that is later than this by adding month_span."

straight-shoota · 2017-10-14T11:48:40Z

src/time.cr

    add_months other.value
  end

-  def -(other : MonthSpan)
+  # Subtracts a number of months specified by *other*'s value.
+  def -(other : Time::MonthSpan) : Time


other => month_span

And doc: "Returns a Time that is earlier than this by subtracting span."

straight-shoota · 2017-10-14T11:49:39Z

src/time.cr

@@ -241,7 +245,8 @@ struct Time
    temp + time_of_day
  end

-  def add_span(seconds, nanoseconds)
+  # Returns a `Time` that is this number of *seconds* and *nanoseconds* later.


"Returns a Time that is later than this by adding seconds and nanoseconds."

straight-shoota · 2017-10-14T11:51:32Z

src/time.cr

@@ -262,7 +267,8 @@ struct Time
    Time.new(seconds: seconds, nanoseconds: nanoseconds.to_i, kind: kind)
  end

-  def -(other : Time)
+  # Returns the amount of time between *other* and `self`.


amount => difference?

straight-shoota · 2017-10-14T11:58:18Z

src/time.cr

@@ -275,76 +281,90 @@ struct Time
    end
  end

-  def self.now : self
+  # Returns the current time in the local time zone.


"Returns the current local time".

Currently time zones are not implemented at all.
Even if, it would be debatable if the local time returned by Time.now would necessarily be in a specific time zone. I'm not sure how this is going to be handled once we get there, but currently there is obviously no time zone attached.

This wording is also used in the doumentation for Go's Time.Now or Python's datetime.now

straight-shoota · 2017-10-14T12:07:53Z

src/time.cr

    seconds, nanoseconds = compute_seconds_and_nanoseconds
    new(seconds: seconds, nanoseconds: nanoseconds, kind: Kind::Utc)
  end

-  def date
+  # Returns a copy of `self` with time-of-day components (hour, minute, ...) set to zero.


Why not name it explicitly hour, minute, second, nanosecond?

oprypin · 2017-10-14T12:38:58Z

I feel like at this point I'm just annoyed with the comments even if they are good.
This is ready to merge :s

RX14 · 2017-10-14T12:43:45Z

src/time.cr

    year_month_day_day_year[2]
  end

-  def hour
+  # Returns the hour of the day (`0...24`).


I'm even more convinced this is bad now.

asterite · 2017-10-14T13:08:57Z

This is ready for merge too for me. About the inclusive/exclusive ranges, both are fine by me. Maybe inclusive is slightly better because if read quickly there's no possibility of confusion.

RX14

Last ones I promise...

RX14 · 2017-10-14T13:55:35Z

src/time.cr

@@ -262,7 +267,8 @@ struct Time
    Time.new(seconds: seconds, nanoseconds: nanoseconds.to_i, kind: kind)
  end

-  def -(other : Time)
+  # Returns the amount of time between *other* and `self`.


My only concern is that this method doesn't make it 100% clear that the time returned can be negative. "The amount of time between" can easily be interpreted as being abs(difference).

Maybe just saying/adding "The amount can be negative if self is a Time that happens before other`

RX14 · 2017-10-14T13:57:39Z

src/time.cr

    ((total_seconds % SECONDS_PER_HOUR) / SECONDS_PER_MINUTE).to_i
  end

-  def second
+  # Returns the second of the minute (`0..59`).


"(0..59, or 0..60 when there's a leap second)"? We mention leap years below so perhaps we should be consistent.

Are you sure 60 can actually be returned? It doesn't look like, seeing that the method does a remainder.

You're correct: https://stackoverflow.com/a/16539734/2035962. Nevermind.

Add documentation and type annotations for Time

0d059ba

bew reviewed Oct 9, 2017

View reviewed changes

RX14 requested changes Oct 9, 2017

View reviewed changes

RX14 reviewed Oct 11, 2017

View reviewed changes

oprypin force-pushed the time-doc branch from 0237fae to 5973729 Compare October 11, 2017 23:01

Reword +/-

adccf9d

oprypin force-pushed the time-doc branch from 5973729 to adccf9d Compare October 12, 2017 10:31

RX14 requested changes Oct 13, 2017

View reviewed changes

Reword

ec2454f

RX14 approved these changes Oct 13, 2017

View reviewed changes

asterite reviewed Oct 13, 2017

View reviewed changes

src/time/span.cr

end

def duration

# Alias of `abs`.

def duration : Time::Span

Copy link

Member

asterite Oct 13, 2017

Choose a reason for hiding this comment

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

We can probably remove this alias in a later PR.

asterite approved these changes Oct 13, 2017

View reviewed changes

Clarify 'converts'

706317d

bew reviewed Oct 13, 2017

View reviewed changes

straight-shoota reviewed Oct 14, 2017

View reviewed changes

RX14 requested changes Oct 14, 2017

View reviewed changes

Inclusive ranges [skip ci]

049f319

asterite approved these changes Oct 14, 2017

View reviewed changes

RX14 reviewed Oct 14, 2017

View reviewed changes

Negative difference [skip ci]

9f63a3d

RX14 approved these changes Oct 14, 2017

View reviewed changes

RX14 merged commit 3e14424 into crystal-lang:master Oct 14, 2017

RX14 added this to the Next milestone Oct 14, 2017

RX14 added the kind:docs label Oct 14, 2017

lipanski mentioned this pull request Nov 18, 2017

Prepare for Crystal 0.24 imdrasil/jennifer.cr#84

Merged

asterite mentioned this pull request Jun 26, 2018

Remove singular names like Int#day, Int#month, etc. #6273

Closed

Add documentation and type annotations for Time #5093

Add documentation and type annotations for Time #5093

Conversation

oprypin commented Oct 9, 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

Choose a reason for hiding this comment

oprypin Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

RX14 Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oprypin commented Oct 12, 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

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

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

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

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

RX14 commented Oct 13, 2017

straight-shoota left a 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

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

oprypin commented Oct 14, 2017

Choose a reason for hiding this comment

asterite commented Oct 14, 2017

RX14 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asterite Oct 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 Oct 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 Oct 14, 2017 • edited Loading

Choose a reason for hiding this comment

oprypin Oct 10, 2017 •

edited

Loading

RX14 Oct 11, 2017 •

edited

Loading

RX14 Oct 11, 2017 •

edited

Loading

RX14 left a comment •

edited

Loading

asterite Oct 14, 2017 •

edited

Loading

RX14 Oct 14, 2017 •

edited

Loading

RX14 Oct 14, 2017 •

edited

Loading