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

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

YAML/Time parsers/formatters now take in care nanoseconds.

Fixes intersection of #5007 and #5022, fixes some issues of #5022, extends #5069.

P.S.: "%N" format specifier added to provide nanoseconds.

straight-shoota · 2017-10-02T22:24:24Z

src/time/format/formatter.cr

@@ -216,5 +220,30 @@ struct Time::Format
      io.write_byte padding.ord.to_u8 if value < 1000
      pad3 value, padding
    end
+
+    def pad5(value, padding)


What are pad5 trough 8 for? I don't see any usecase for these. It's certainly easier to use log10 for pad9 instead of dereasingly calling these methods.

Yes, it look like strange code duplication because numbers aren't used. Will collapse.

straight-shoota · 2017-10-02T22:28:20Z

src/time/format/parser.cr

@@ -13,7 +13,7 @@ struct Time::Format
      @hour = 0
      @minute = 0
      @second = 0
-      @nanosecond = 0
+      @nanosecond = 0i64


This shouldn't be Int64, Time#@nanosecond is only Int32.

Just looked at any code piece - NANOSECONDS_PER_MILLISECOND = 1_000_000_i64.

Ok, will fix :)

straight-shoota · 2017-10-02T22:30:41Z

This is not just a fix but an additional feature, enabling nanosecond precision for time formats and YAML serializer.

Do you know why the specs in core_spec.cr didn't fail? They're missing the same quantifier as the one in serialization_spec.cr...

akzhan · 2017-10-02T22:51:26Z

@straight-shoota looks like it_parses_scalar in specs check Time values with minimal precision.

Sija · 2017-10-02T22:52:29Z

src/time/format.cr

@@ -28,6 +28,7 @@
 # * **%k**: hour of the day, 24-hour clock, blank padded (" 0", " 1", ..., "24")
 # * **%l**: hour of the day, 12-hour clock, blank padded (" 0", " 1", ..., "12")
 # * **%L**: milliseconds, zero padded (000, 001, ..., 999)
+# * **%N**: nanoseconds, zero padded (000000000, 000000001, ...)


Please, add the last value after the ellipsis to mark complete range, like in the other examples.

Thanks, @Sija.

asterite · 2017-10-03T12:39:40Z

spec/std/yaml/schema/core_spec.cr

-  it_parses_scalar "2002-1-2T10:11:12.34", Time.new(2002, 1, 2, 10, 11, 12, 340, kind: Time::Kind::Utc)
-  it_parses_scalar "2002-1-2T10:11:12.345", Time.new(2002, 1, 2, 10, 11, 12, 345, kind: Time::Kind::Utc)
-  it_parses_scalar "2002-1-2T10:11:12.3456", Time.new(2002, 1, 2, 10, 11, 12, 345, kind: Time::Kind::Utc)
+  it_parses_scalar "2002-1-2T10:11:12.3", Time.new(2002, 1, 2, 10, 11, 12, 300 * Time::NANOSECONDS_PER_MILLISECOND, kind: Time::Kind::Utc)


Please do not use these :nodoc: constants from Time, they are nodoc for a reason.

NANOSECONDS_PER_MILLISECOND dropped from specs.

asterite · 2017-10-03T12:59:31Z

spec/std/yaml/schema/core_spec.cr

+  it_parses_scalar "2002-1-2T10:11:12.3", Time.new(2002, 1, 2, 10, 11, 12, 300 * Time::NANOSECONDS_PER_MILLISECOND, kind: Time::Kind::Utc)
+  it_parses_scalar "2002-1-2T10:11:12.34", Time.new(2002, 1, 2, 10, 11, 12, 340 * Time::NANOSECONDS_PER_MILLISECOND, kind: Time::Kind::Utc)
+  it_parses_scalar "2002-1-2T10:11:12.345", Time.new(2002, 1, 2, 10, 11, 12, 345 * Time::NANOSECONDS_PER_MILLISECOND, kind: Time::Kind::Utc)
+  it_parses_scalar "2002-1-2T10:11:12.3456", Time.new(2002, 1, 2, 10, 11, 12, 345.6 * Time::NANOSECONDS_PER_MILLISECOND, kind: Time::Kind::Utc)


You can just pass 345_600_000 here

akzhan · 2017-10-03T20:32:08Z

/cc @ysbaddaden, @mverzilli i'll be glad to see this fix'n'feature merged.

straight-shoota · 2017-10-04T08:45:36Z

src/time/format/parser.cr

      pos = @reader.pos
-      millisecond = consume_number(12)
+      millisecond = consume_number_i64(12)


I don't think there is a need for 64 bit integer here. You should use consume_number. It will still read 12 digits.

this value will be multiplied by Int64 later. Less conversions needed.

consume_number is just consume_number_i64.to_i.

parsing of 12 numbers at once is just current reasonable choice (picoseconds). It may be changed later.

straight-shoota · 2017-10-04T08:48:11Z

src/time/format/parser.cr

+      # and later just use the first 9 digits because Time
+      # only has nanosecond precision.
+      pos = @reader.pos
+      nanosecond = consume_number_i64(12)


Here I'm wrong and will replace with consume_number, thanks.

akzhan · 2017-10-04T17:20:14Z

@bcardiff can you just merge this?

my code fails unless formatters and parsers fixed.

bcardiff · 2017-10-04T17:36:22Z

@akzhan I think first we need to wait/merge #5072

akzhan · 2017-10-04T21:01:17Z

@bcardiff they are independent. moveover I think that #5072 is step aside.

bcardiff · 2017-10-05T13:47:25Z

@akzhan #5072 landed to master now, the conflicts can be fixed and this PR can evolve.

From ruby I've found that the %N may support different precision. The default is %N = %9N = nanosecond so it's safe to just implement %N in this PR.

    %N - Fractional seconds digits, default is 9 digits (nanosecond)
            %3N  millisecond (3 digits)   %15N femtosecond (15 digits)
            %6N  microsecond (6 digits)   %18N attosecond  (18 digits)
            %9N  nanosecond  (9 digits)   %21N zeptosecond (21 digits)

…stead-of-milliseconds

akzhan · 2017-10-05T14:25:30Z

@bcardiff merge conflicts resolved.

Yes, I was implemented %N with default precision to keep things simple.

straight-shoota · 2017-10-05T15:15:07Z

@akzhan specs are failing

Sija · 2017-10-05T15:40:29Z

src/yaml/schema/core/time_parser.cr

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

    return nil if @reader.has_next?

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


should be nanosecond: nanosecond.

akzhan · 2017-10-05T17:38:04Z

Thanks @straight-shoota and @Sija for your attention.

asterite

If I'm not mistaken, a spec to prove that %N works well is missing. You should add one in spec/std/time/time_spec.cr.

akzhan · 2017-10-05T20:10:37Z

@asterite Done.

bcardiff · 2017-10-05T20:16:01Z

spec/std/yaml/schema/core_spec.cr

@@ -117,7 +117,7 @@ describe YAML::Schema::Core do
  it_parses_scalar "2002-1-2T10:11:12.3", Time.new(2002, 1, 2, 10, 11, 12, nanosecond: 300_000_000, kind: Time::Kind::Utc)
  it_parses_scalar "2002-1-2T10:11:12.34", Time.new(2002, 1, 2, 10, 11, 12, nanosecond: 340_000_000, kind: Time::Kind::Utc)
  it_parses_scalar "2002-1-2T10:11:12.345", Time.new(2002, 1, 2, 10, 11, 12, nanosecond: 345_000_000, kind: Time::Kind::Utc)
-  it_parses_scalar "2002-1-2T10:11:12.3456", Time.new(2002, 1, 2, 10, 11, 12, nanosecond: 345_000_000, kind: Time::Kind::Utc)
+  it_parses_scalar "2002-1-2T10:11:12.3456", Time.new(2002, 1, 2, 10, 11, 12, nanosecond: 345_600_000, kind: Time::Kind::Utc)


For this spec and for the Time.parse one I think there should be better coverage. The pad9 method has some branches that are not stressed right now.

Hey, I just fixes the bugs. padX is very implementation details and I never will test 'em.

Nobody tests pad2..4...

akzhan mentioned this pull request Oct 2, 2017

Fix for YAML spec failure with new nanoseconds precision #5069

Merged

straight-shoota requested changes Oct 2, 2017

View reviewed changes

Sija reviewed Oct 2, 2017

View reviewed changes

akzhan force-pushed the time-constructor=now-expect-nanoseconds-instead-of-milliseconds branch from 24b9166 to d039e04 Compare October 2, 2017 22:54

asterite requested changes Oct 3, 2017

View reviewed changes

asterite approved these changes Oct 3, 2017

View reviewed changes

asterite mentioned this pull request Oct 3, 2017

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

Merged

akzhan force-pushed the time-constructor=now-expect-nanoseconds-instead-of-milliseconds branch 2 times, most recently from 58b284b to 7c5bc6c Compare October 3, 2017 15:07

akzhan changed the title ~~Time constructor now expect nanoseconds instead of milliseconds.~~ YAML/Time parsers/formatters now take in care nanoseconds Oct 4, 2017

akzhan force-pushed the time-constructor=now-expect-nanoseconds-instead-of-milliseconds branch from 7c5bc6c to af682c0 Compare October 4, 2017 07:28

straight-shoota reviewed Oct 4, 2017

View reviewed changes

akzhan added 2 commits October 4, 2017 12:22

YAML/Time parsers/formatters now take in care nanoseconds.

3f781ef

Describe N format specifier for nanoseconds.

7880992

akzhan force-pushed the time-constructor=now-expect-nanoseconds-instead-of-milliseconds branch from 7432972 to 7880992 Compare October 4, 2017 09:23

Merge branch 'master' into time-constructor=now-expect-nanoseconds-in…

52a6d14

…stead-of-milliseconds

Sija reviewed Oct 5, 2017

View reviewed changes

fix merge conflict (@Sija)

fe1c973

fix spec

ae19379

asterite requested changes Oct 5, 2017

View reviewed changes

Spec for %N format specifier.

1d060b8

bcardiff reviewed Oct 5, 2017

View reviewed changes

asterite approved these changes Oct 5, 2017

View reviewed changes

RX14 approved these changes Oct 5, 2017

View reviewed changes

RX14 added kind:feature topic:stdlib labels Oct 5, 2017

RX14 added this to the Next milestone Oct 5, 2017

RX14 merged commit bdd038d into crystal-lang:master Oct 5, 2017

akzhan deleted the time-constructor=now-expect-nanoseconds-instead-of-milliseconds branch October 5, 2017 22:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

akzhan commented Oct 2, 2017 •

edited

straight-shoota Oct 2, 2017

akzhan Oct 2, 2017

straight-shoota Oct 2, 2017

akzhan Oct 2, 2017

straight-shoota commented Oct 2, 2017

akzhan commented Oct 2, 2017

Sija Oct 2, 2017

akzhan Oct 2, 2017

asterite Oct 3, 2017

akzhan Oct 3, 2017

asterite Oct 3, 2017

akzhan Oct 3, 2017

akzhan commented Oct 3, 2017

straight-shoota Oct 4, 2017 •

edited

akzhan Oct 4, 2017 •

edited

straight-shoota Oct 4, 2017

akzhan Oct 4, 2017

akzhan commented Oct 4, 2017

bcardiff commented Oct 4, 2017

akzhan commented Oct 4, 2017

bcardiff commented Oct 5, 2017

akzhan commented Oct 5, 2017

straight-shoota commented Oct 5, 2017

Sija Oct 5, 2017

akzhan commented Oct 5, 2017

asterite left a comment

akzhan commented Oct 5, 2017

bcardiff Oct 5, 2017

akzhan Oct 5, 2017 •

edited

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

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

Conversation

akzhan commented Oct 2, 2017 • edited

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 2, 2017

akzhan commented Oct 2, 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

akzhan commented Oct 3, 2017

straight-shoota Oct 4, 2017 • edited

Choose a reason for hiding this comment

akzhan Oct 4, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akzhan commented Oct 4, 2017

bcardiff commented Oct 4, 2017

akzhan commented Oct 4, 2017

bcardiff commented Oct 5, 2017

akzhan commented Oct 5, 2017

straight-shoota commented Oct 5, 2017

Choose a reason for hiding this comment

akzhan commented Oct 5, 2017

asterite left a comment

Choose a reason for hiding this comment

akzhan commented Oct 5, 2017

Choose a reason for hiding this comment

akzhan Oct 5, 2017 • edited

Choose a reason for hiding this comment

akzhan commented Oct 2, 2017 •

edited

straight-shoota Oct 4, 2017 •

edited

akzhan Oct 4, 2017 •

edited

akzhan Oct 5, 2017 •

edited