-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
YAML/Time parsers/formatters now take in care nanoseconds #5070
Conversation
src/time/format/formatter.cr
Outdated
@@ -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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
src/time/format/parser.cr
Outdated
@@ -13,7 +13,7 @@ struct Time::Format | |||
@hour = 0 | |||
@minute = 0 | |||
@second = 0 | |||
@nanosecond = 0 | |||
@nanosecond = 0i64 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
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 |
@straight-shoota looks like |
src/time/format.cr
Outdated
@@ -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, ...) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @Sija.
24b9166
to
d039e04
Compare
spec/std/yaml/schema/core_spec.cr
Outdated
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
spec/std/yaml/schema/core_spec.cr
Outdated
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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
58b284b
to
7c5bc6c
Compare
/cc @ysbaddaden, @mverzilli i'll be glad to see this fix'n'feature merged. |
7c5bc6c
to
af682c0
Compare
pos = @reader.pos | ||
millisecond = consume_number(12) | ||
millisecond = consume_number_i64(12) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- this value will be multiplied by
Int64
later. Less conversions needed. consume_number
is justconsume_number_i64.to_i
.- parsing of 12 numbers at once is just current reasonable choice (picoseconds). It may be changed later.
src/time/format/parser.cr
Outdated
# and later just use the first 9 digits because Time | ||
# only has nanosecond precision. | ||
pos = @reader.pos | ||
nanosecond = consume_number_i64(12) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto.
There was a problem hiding this comment.
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.
7432972
to
7880992
Compare
@bcardiff can you just merge this? my code fails unless formatters and parsers fixed. |
@akzhan #5072 landed to master now, the conflicts can be fixed and this PR can evolve. From ruby I've found that the
|
…stead-of-milliseconds
@bcardiff merge conflicts resolved. Yes, I was implemented %N with default precision to keep things simple. |
@akzhan specs are failing |
src/yaml/schema/core/time_parser.cr
Outdated
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be nanosecond: nanosecond
.
Thanks @straight-shoota and @Sija for your attention. |
There was a problem hiding this 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
.
@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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
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.