Extend documentation of Time and Time::Location APIs #5819

straight-shoota · 2018-03-13T00:10:13Z

This PR adds extended documentation for most public types and methods of the Time and Time::Location API.

There are no code changes except from adding a few type restrictions. The constructors of Time have been aligned in a more logical order (although this is only relevant in the source code, not the API docs), but there are no changes to the actual implementation.

I'm pretty sure the documentation is generally correct. But it's a lot of content and I expect there to be semantic mistakes, abstruse descriptions as well as grammatical, stylistic and typographical errors. Please review thoroughly and point me to where I need to improve.

bew · 2018-03-13T01:40:28Z

src/time.cr

+  # Measures the execution time of *block*.
+  #
+  # The measurement relies on the monotonic clock and is not
+  # affected by fluctuations o the system clock (see `#monotonic`).


o => of

bew · 2018-03-13T01:47:51Z

src/time.cr

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


Can you re-add the hint about what time-of-day components are? (the (hour, minute, ...))

bew · 2018-03-13T01:50:29Z

src/time.cr

+  # time_ar = Time.new(2018, 3, 8, 18, 5, 13, location: Time::Location.load("America/Buenos_Aires"))
+  # time_de == time_ar # => true
+  #
+  # both times represent the same instant:


Put this line in comment?

# # both ...

bew · 2018-03-13T01:54:04Z

src/time.cr

-  # Parses a Time in the given *time* string, using the given *pattern* (see
-  # `Time::Format`).
+  # Parses a `Time` from *time* string using the given *pattern* (see
+  # `Time::Format` for details).


Note: For the 2 methods above you have the See `Time::Format` for details on a new line, here it is in parentheses on the same line as the description. I think we should keep consistency and put this on a new line.

bew · 2018-03-13T01:58:01Z

src/time.cr

@@ -561,7 +903,10 @@ struct Time
    )
  end

-  # Returns a copy of this `Time` converted to UTC.
+  # Returns a copy of this `Time` representing the same instant in UTC


in UTC time zone ?

I think it's unneccessary. UTC implies it is a time zone and is never written that explicitly in other places.

bew · 2018-03-13T01:58:58Z

src/time.cr

@@ -606,7 +954,9 @@ struct Time
  def_at_beginning(hour) { Time.new(year, month, day, hour, location: location) }
  def_at_beginning(minute) { Time.new(year, month, day, hour, minute, location: location) }

-  # Returns the time when the week that includes `self` starts.
+  # Returns a copy of this `Time` representing the begin of the week.


begin => beginning ?

bew · 2018-03-13T02:04:16Z

src/time/location.cr

    def initialize(@name : String, @offset : Int32, @dst : Bool)
-      # Maximium offets of IANA timezone database are -12:00 and +14:00.
+      # Maximium offets of IANA time zone database are -12:00 and +14:00.


offets => offsets ?

bew · 2018-03-13T02:16:00Z

src/time/location.cr

  def lookup(time : Time) : Zone
    lookup(time.epoch)
  end

-  # Returns the time zone in use at `epoch` (time in seconds since UNIX epoch).
+  # Returns the time zone offset observed at *epoch*.
+  # *epoch* expresses the number of seconds since UNIX epoch


Blank line before this line?

straight-shoota · 2018-03-13T11:41:33Z

Thanks @bew!

straight-shoota · 2018-03-13T12:21:42Z

I intentionally used additional indent for comments in the code examples to align them across different lines and code blocks. Unfortunately, the formatter doesn't allow that and I had to adjust to comply with the formatter rules (second commit). There should be a way to keep the intended formatting to improve readability.

Sija · 2018-03-13T12:43:13Z

src/time/location/loader.cr

    file.seek -ZIP_TAIL_SIZE, IO::Seek::End

    if file.read_bytes(Int32, IO::ByteFormat::LittleEndian) != END_OF_CENTRAL_DIRECTORY_HEADER_SIGNATURE
-      raise InvalidTZDataError.new("corrupt zip file")
+      raise InvalidTZDataError.new("corrupt zip file #{file.path}")


Please, use "Sentence case" here. ditto all below.

RX14 · 2018-03-13T15:05:06Z

src/time.cr

@@ -1,66 +1,206 @@
 require "crystal/system/time"

-# `Time` represents an instance in incremental time. Here are some examples:
+# `Time` represents a date-time instant in incremental time observed in


It's not incremental time, that guarantee is reserved for Time.monotonic. I'd prefer the wording "an instant in time observed in a specific time zone".

Time has an incremental time-line, which does not necessarily mean a monotonic clock. In fact, it has little to do with that.

"Incremental" means each instance represent a specific moment on the time-line, as opposed to "floating time" which has no time-line at all, or a local date-time which can be ambiguous.

Maybe I should add some reference, for example this link: https://www.w3.org/International/articles/definitions-time/#incremental_time

That link should be in the type docs. I think most of us have no idea what incremental time is.

RX14 · 2018-03-13T15:06:55Z

src/time.cr

+# nanosecond-of-second with value range `0..999_999_999`.
+#
+# The supported date range is `0001-01-01 00:00:00.0` to
+# 999-12-31 23:59:59.999_999_999` in any local time zone.


I hope it's not, since we're already outside that date range!

RX14 · 2018-03-13T15:16:25Z

src/time.cr

+  # and *nanoseconds* elapsed from epoch (`0001-01-01 00:00:00.0 UTC`)
+  # observed in *location*.
+  #
+  # Valid range for *seconds* is `0..315_537_897_599`.


Is there a constant for the maximum number of seconds? There should be and it should be quoted instead of a bare number.

Yes, Time::MAX_SECONDS. But it is not documented.

I agree that it should be documented. And there should probably be Time::MIN_SECONDS = 0 as well.

RX14 · 2018-03-13T15:19:19Z

src/time.cr

+  # Valid range for *seconds* is `0..315_537_897_599`.
+  # For *nanoseconds* it is `0..999_999_999`.
+  def initialize(*, @seconds : Int64, @nanoseconds : Int32, @location : Location)
+    unless 0 <= @nanoseconds < NANOSECONDS_PER_SECOND


BTW does this method not range check @seconds???

Nope. But #5786 is just waiting for a second review =)

Nice pun ;-)

RX14 · 2018-03-13T15:20:23Z

src/time.cr

+  # The duration amounts to the actual time elapsed between both instances, on
+  # the instant time-line.
+  # The difference between local date-time representations may equal to a
+  # differnt duration, depending on time zone transitions.


And leap seconds.

differnt => different

Which are ignored - or, more exactly, are not even recognized - so they shouldn't have any influence.

To be more clear: Neither the instance time-line nor the date-time representations - which are both just models used by Time - take leap seconds into account. Thus leap seconds have no impact on the difference between both.

Leap seconds can only lead to a difference (of one second) to the actual time elapsed.

RX14 · 2018-03-13T15:24:04Z

src/time.cr

    Format.new(pattern, location).parse(time)
  end

-  # Returns the number of seconds since the Epoch for this time.
+  # Returns the number of seconds since Unix epoch (`1970-01-01 00:00:00 UTC`).


the unix epoch and ditto throughout.

jkthorne · 2018-03-16T18:21:40Z

Thank you for all the docs.

straight-shoota · 2018-04-26T12:38:39Z

@RX14 Care for another review?

RX14 · 2018-04-26T14:19:18Z

This has conflicts, and there's no fixup commit with just the changes :(

straight-shoota · 2018-04-26T15:54:34Z

Well, you're responsible for the conflict by merging #5794 😁

It seems like I already merged in the fixups, but I basically applied the changes you requested in the last review or answered to your comments.

Rebased.

bew · 2018-06-04T04:02:03Z

GTG?

straight-shoota · 2018-06-04T21:53:22Z

Rebased

bew reviewed Mar 13, 2018

View reviewed changes

straight-shoota force-pushed the jm/docs/time branch 2 times, most recently from 68f31cf to f4107d0 Compare March 13, 2018 12:17

Sija reviewed Mar 13, 2018

View reviewed changes

straight-shoota force-pushed the jm/docs/time branch 2 times, most recently from adc012e to 89c868c Compare March 13, 2018 15:25

RX14 requested changes Mar 13, 2018

View reviewed changes

straight-shoota force-pushed the jm/docs/time branch 2 times, most recently from 9cdb463 to bfc1ed3 Compare March 13, 2018 20:32

straight-shoota force-pushed the jm/docs/time branch from bfc1ed3 to 700e8ee Compare April 13, 2018 12:54

straight-shoota force-pushed the jm/docs/time branch from 700e8ee to 6fab9e4 Compare April 26, 2018 15:52

RX14 approved these changes Apr 26, 2018

View reviewed changes

straight-shoota force-pushed the jm/docs/time branch from fd2f5a7 to 5bd1129 Compare April 29, 2018 00:21

straight-shoota and others added 5 commits June 4, 2018 23:25

Extend documentation of Time and Time::Location APIs

b1cf6ee

Adjust example comment indent to comply with formatter rules

b7aed85

fixup! Extend documentation of Time and Time::Location APIs

36c30e8

fixup! Extend documentation of Time and Time::Location APIs

1b10d88

fixup! Extend documentation of Time and Time::Location APIs

e66f207

straight-shoota force-pushed the jm/docs/time branch from 5bd1129 to e66f207 Compare June 4, 2018 21:26

RX14 requested a review from bcardiff June 4, 2018 21:55

Fix typo

133462e

bcardiff approved these changes Jun 7, 2018

View reviewed changes

bcardiff merged commit 75f4a61 into crystal-lang:master Jun 7, 2018

bcardiff added this to the 0.25.0 milestone Jun 7, 2018

bcardiff added the kind:docs label Jun 7, 2018

straight-shoota deleted the jm/docs/time branch June 7, 2018 15:35

RX14 mentioned this pull request Jun 12, 2018

Fix Time on win32 #6181

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend documentation of Time and Time::Location APIs #5819

Extend documentation of Time and Time::Location APIs #5819

straight-shoota commented Mar 13, 2018 •

edited

Loading

bew Mar 13, 2018

bew Mar 13, 2018

bew Mar 13, 2018

bew Mar 13, 2018

bew Mar 13, 2018

straight-shoota Mar 13, 2018

bew Mar 13, 2018

bew Mar 13, 2018

bew Mar 13, 2018

straight-shoota commented Mar 13, 2018

straight-shoota commented Mar 13, 2018

Sija Mar 13, 2018 •

edited

Loading

RX14 Mar 13, 2018

straight-shoota Mar 13, 2018 •

edited

Loading

asterite Apr 28, 2018

RX14 Mar 13, 2018

RX14 Mar 13, 2018

straight-shoota Mar 13, 2018

straight-shoota Mar 13, 2018 •

edited

Loading

RX14 Mar 13, 2018

straight-shoota Mar 13, 2018

asterite Mar 13, 2018

RX14 Mar 13, 2018

bew Mar 13, 2018

straight-shoota Mar 13, 2018

straight-shoota Mar 13, 2018 •

edited

Loading

RX14 Mar 13, 2018

jkthorne commented Mar 16, 2018

straight-shoota commented Apr 26, 2018

RX14 commented Apr 26, 2018

straight-shoota commented Apr 26, 2018

bew commented Jun 4, 2018

straight-shoota commented Jun 4, 2018

Extend documentation of Time and Time::Location APIs #5819

Extend documentation of Time and Time::Location APIs #5819

Conversation

straight-shoota commented Mar 13, 2018 • 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

Choose a reason for hiding this comment

straight-shoota commented Mar 13, 2018

straight-shoota commented Mar 13, 2018

Sija Mar 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

straight-shoota Mar 13, 2018 • 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

straight-shoota Mar 13, 2018 • 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

straight-shoota Mar 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkthorne commented Mar 16, 2018

straight-shoota commented Apr 26, 2018

RX14 commented Apr 26, 2018

straight-shoota commented Apr 26, 2018

bew commented Jun 4, 2018

straight-shoota commented Jun 4, 2018

straight-shoota commented Mar 13, 2018 •

edited

Loading

Sija Mar 13, 2018 •

edited

Loading

straight-shoota Mar 13, 2018 •

edited

Loading

straight-shoota Mar 13, 2018 •

edited

Loading

straight-shoota Mar 13, 2018 •

edited

Loading