artichoke / strftime-ruby

⏳ Ruby `Time#strftime` parser and formatter
https://crates.io/crates/strftime-ruby
MIT License
12 stars 0 forks source link

Implement strftime #12

Closed x-hgg-x closed 2 years ago

x-hgg-x commented 2 years ago

Implement strftime.

x-hgg-x commented 2 years ago

Yes, I use the specification at https://ruby-doc.org/core-3.1.2/Time.html#method-i-strftime, with verification using the interpreter.

lopopolo commented 2 years ago

Awesome thank you!

x-hgg-x commented 2 years ago

Potential inconsistency with Ruby 3.1.2:

Time.new(2021,10,21,1,1,1,'Z').wday # 7 (Sunday)
Time.at(Time.new(2021,10,21,1,1,1,'Z'), in: 'Z').wday # 4 (Thursday)
x-hgg-x commented 2 years ago

It seems that many specifiers are invalid when the time zone is UTC:

Time.new(2019,12,31,12,1,1,'Z').strftime("%G %g %V %U %W %u %w %j %a %A") # "2019 19 00 00 00 7 6 001 ? ?"
Time.new(2019,12,31,12,1,1,'A').strftime("%G %g %V %U %W %u %w %j %a %A") # "2020 20 01 52 52 2 2 365 Tue Tuesday"
lopopolo commented 2 years ago

It seems that many specifiers are invalid when the time zone is UTC:

Time.new(2019,12,31,12,1,1,'Z').strftime("%G %g %V %U %W %u %w %j %a %A") # "2019 19 00 00 00 7 6 001 ? ?"
Time.new(2019,12,31,12,1,1,'A').strftime("%G %g %V %U %W %u %w %j %a %A") # "2020 20 01 52 52 2 2 365 Tue Tuesday"

yea something weird is going on there:

[3.1.2] > Time.new(2019,12,31,12,1,1,'Z').asctime
=> "? Dec 31 12:01:01 2019"
[3.1.2] > Time.new(2019,12,31,12,1,1,'A').asctime
=> "Tue Dec 31 12:01:01 2019"
lopopolo commented 2 years ago

Potential inconsistency with Ruby 3.1.2:

Time.new(2021,10,21,1,1,1,'Z').wday # 7 (Sunday)
Time.at(Time.new(2021,10,21,1,1,1,'Z'), in: 'Z').wday # 4 (Thursday)

Adding some more examples of this weirdness:

[3.1.2] > Time.new(2021,10,21,1,1,1,'Z').asctime
=> "? Oct 21 01:01:01 2021"
[3.1.2] > Time.at(Time.new(2021,10,21,1,1,1,'Z'), in: 'Z').asctime
=> "Thu Oct 21 01:01:01 2021"
[3.1.2] > Time.new(2021,10,21,1,1,1, in: 'Z').asctime
=> "? Oct 21 01:01:01 2021"
[3.1.2] > Time.at(Time.new(2021,10,21,1,1,1,in: 'Z'), in: 'Z').asctime
=> "Thu Oct 21 01:01:01 2021"
lopopolo commented 2 years ago

yea something weird is going on there

I'm taking a guess here but I bet Time::new manually populates the tm struct from time.h whereas Time::at actually does timezone resolution.

Regardless, I think what spinoso-time and tz-rs + tzdb are doing is more correct and I think it's too much complexity to emulate MRI's (broken) behavior.

x-hgg-x commented 2 years ago

The doc of spinoso_time::tzrs::Time::day_of_year() is inconsistent with its implementation: https://github.com/artichoke/artichoke/blob/b3284a42fb0d175da1a822bda73bd300ddab72c3/spinoso-time/src/time/tzrs/parts.rs#L511-L533.

tz::datetime::DateTime returns a year day in [0, 365], but the spinoso_time doc say it should be in 1..366 (1..=366 is more correct I think).

lopopolo commented 2 years ago

The doc of spinoso_time::tzrs::Time::day_of_year() is inconsistent with its implementation: https://github.com/artichoke/artichoke/blob/b3284a42fb0d175da1a822bda73bd300ddab72c3/spinoso-time/src/time/tzrs/parts.rs#L511-L533.

tz::datetime::DateTime returns a year day in [0, 365], but the spinoso_time doc say it should be in 1..366 (1..=366 is more correct I think).

Bug and PR to fix this:

lopopolo commented 2 years ago

Fix is merged in https://github.com/artichoke/artichoke/commit/17e469b0ea30d235e90c5464a1e7f38ca316d7c4.

x-hgg-x commented 2 years ago

The Unix timestamp is needed for the %s format. Corresponding issue: https://github.com/artichoke/artichoke/issues/2074.

lopopolo commented 2 years ago

I can prep the release after the tests pass and this gets merged. ooc, given that a trait is part of the public API, should we release as 1.0.0?

x-hgg-x commented 2 years ago

I think the API is pretty complete at this point, so this should be ok.

lopopolo commented 2 years ago

🎉

@x-hgg-x thank you again so much. I'll make you a maintainer here later today when I'm done with work.

There are a few changes I'd like to try and make, e.g. removing the bitflags dep. I'll tag you as a reviewer.

Hopefully we can push out a 1.0 by the end of this week or next.