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

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

Conversation

akzhan
Copy link
Contributor

@akzhan akzhan commented Oct 2, 2017

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.

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Ok, will fix :)

@straight-shoota
Copy link
Member

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

akzhan commented Oct 2, 2017

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @Sija.

@akzhan akzhan force-pushed the time-constructor=now-expect-nanoseconds-instead-of-milliseconds branch from 24b9166 to d039e04 Compare October 2, 2017 22:54
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)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NANOSECONDS_PER_MILLISECOND dropped from specs.

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

Choose a reason for hiding this comment

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

You can just pass 345_600_000 here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

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

akzhan commented Oct 3, 2017

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

@akzhan 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 akzhan force-pushed the time-constructor=now-expect-nanoseconds-instead-of-milliseconds branch from 7c5bc6c to af682c0 Compare October 4, 2017 07:28
pos = @reader.pos
millisecond = consume_number(12)
millisecond = consume_number_i64(12)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@akzhan akzhan Oct 4, 2017

Choose a reason for hiding this comment

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

  1. this value will be multiplied by Int64 later. Less conversions needed.
  2. consume_number is just consume_number_i64.to_i.
  3. parsing of 12 numbers at once is just current reasonable choice (picoseconds). It may be changed later.

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

Choose a reason for hiding this comment

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

ditto.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

akzhan commented Oct 4, 2017

@bcardiff can you just merge this?

my code fails unless formatters and parsers fixed.

@bcardiff
Copy link
Member

bcardiff commented Oct 4, 2017

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

@akzhan
Copy link
Contributor Author

akzhan commented Oct 4, 2017

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

@bcardiff
Copy link
Member

bcardiff commented Oct 5, 2017

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

@akzhan
Copy link
Contributor Author

akzhan commented Oct 5, 2017

@bcardiff merge conflicts resolved.

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

@straight-shoota
Copy link
Member

@akzhan specs are failing

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

Choose a reason for hiding this comment

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

should be nanosecond: nanosecond.

@akzhan
Copy link
Contributor Author

akzhan commented Oct 5, 2017

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

Copy link
Member

@asterite asterite left a comment

Choose a reason for hiding this comment

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

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

akzhan commented Oct 5, 2017

@asterite Done.

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@akzhan akzhan Oct 5, 2017

Choose a reason for hiding this comment

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

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

Nobody tests pad2..4...

RX14
RX14 approved these changes Oct 5, 2017
@RX14 RX14 added this to the Next milestone Oct 5, 2017
@RX14 RX14 merged commit bdd038d into crystal-lang:master Oct 5, 2017
@akzhan akzhan deleted the time-constructor=now-expect-nanoseconds-instead-of-milliseconds branch October 5, 2017 22:15
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

6 participants