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

Add documentation and type annotations for Time #5093

Merged
merged 6 commits into from Oct 14, 2017

Conversation

oprypin
Copy link
Member

@oprypin oprypin commented Oct 9, 2017

No description provided.

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not keep self here?

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. If something subclasses Time, this will not hold anymore (uhh subclassing a struct is impossible, but the theoretic point still stands)
  2. It can be confused with actually returning self (but modified). This way it's clear that it's an entirely new instance.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. 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)
  2. 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

src/time.cr Outdated
self
end

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

Choose a reason for hiding this comment

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

s/this/a/? hmm.

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

Copy link
Member Author

@oprypin oprypin Oct 10, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@RX14 RX14 Oct 11, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure

src/time.cr Outdated
@@ -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.
Copy link
Contributor

@RX14 RX14 Oct 11, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, reverted that

@oprypin
Copy link
Member Author

oprypin commented Oct 12, 2017

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

src/time.cr Outdated
@@ -275,76 +281,90 @@ struct Time
end
end

def self.now : self
# Returns the current time in the current time zone.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

OK

src/time.cr Outdated
Time.new(year, month, day, kind: kind)
end

def year
# Returns the year number (Common Era).
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

OK

src/time.cr Outdated
year_month_day_day_year[1]
end

def day
# Returns the day number of the month (`0..31`).
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

OK

src/time.cr Outdated
year_month_day_day_year[2]
end

def hour
# Returns the hour of the day (`0...24`).
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 ..

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm even more convinced this is bad now.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

src/time.cr Outdated
(total_seconds % SECONDS_PER_MINUTE).to_i
end

def millisecond
# Returns the millisecond of the second (`0...1000`).
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto on 0..999.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ditto.

Copy link
Member

Choose a reason for hiding this comment

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

Ditto. 😛

(to_i.to_f * NANOSECONDS_PER_SECOND) + nanoseconds
end

def to_f
total_seconds
# Converts to a number of milliseconds.
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be fractional too, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes but come on, it's to_f

src/time/span.cr Outdated
abs
end

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

OK

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Time.now + self
end

def ago
# Returns a `Time` that happens earlier by `self` than the current time.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

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

struct Int
def week
# :nodoc:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

RX14
RX14 approved these changes Oct 13, 2017
src/time.cr Outdated
@@ -452,7 +474,8 @@ struct Time
epoch.to_f + nanosecond.to_f / 1e9
end

def to_utc
# Converts this time to UTC.
Copy link
Member

Choose a reason for hiding this comment

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

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.

end

def duration
# Alias of `abs`.
def duration : Time::Span
Copy link
Member

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.

src/time.cr Outdated
@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

As a struct, it's still a copy.

Copy link
Member

Choose a reason for hiding this comment

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

@oprypin Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

oh right I forgot that minor fact

@RX14
Copy link
Contributor

RX14 commented Oct 13, 2017

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

Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

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

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*.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

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*.
Copy link
Member

Choose a reason for hiding this comment

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

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

end

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

Choose a reason for hiding this comment

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

other => span (or month_span)

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

add_months other.value
end

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

Choose a reason for hiding this comment

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

other => month_span

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

@@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -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`.
Copy link
Member

Choose a reason for hiding this comment

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

amount => difference?

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

def self.now : self
# Returns the current time in the local time zone.
Copy link
Member

Choose a reason for hiding this comment

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

"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

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.
Copy link
Member

Choose a reason for hiding this comment

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

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

@oprypin
Copy link
Member Author

oprypin commented Oct 14, 2017

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

src/time.cr Outdated
year_month_day_day_year[2]
end

def hour
# Returns the hour of the day (`0...24`).
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm even more convinced this is bad now.

@asterite
Copy link
Member

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.

Copy link
Contributor

@RX14 RX14 left a comment

Choose a reason for hiding this comment

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

Last ones I promise...

@@ -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`.
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Member

@asterite asterite Oct 14, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

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

def second
# Returns the second of the minute (`0..59`).
Copy link
Contributor

@RX14 RX14 Oct 14, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@RX14 RX14 Oct 14, 2017

Choose a reason for hiding this comment

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

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

RX14
RX14 approved these changes Oct 14, 2017
@RX14 RX14 merged commit 3e14424 into crystal-lang:master Oct 14, 2017
@RX14 RX14 added this to the Next milestone Oct 14, 2017
@RX14 RX14 added the kind:docs label Oct 14, 2017
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.

None yet

5 participants