Project

General

Profile

Bug #9836

Bad Implementation of Time.strptime

Added by silverhammermba (Max Anselm) over 5 years ago. Updated 2 months ago.

Status:
Feedback
Priority:
Normal
Assignee:
-
Target version:
-
ruby -v:
ruby 2.1.2p95 (2014-05-08 revision 45877) [x86_64-linux]
[ruby-core:62567]

Description

According to the documentation, Time.strptime "parses +date+ using Date._strptime and converts it to a Time object." However this conversion is extremely naive, using only certain fields return by Date._strptime and ignoring the rest (for example :wnum0).

This creates confusing and inconsistent behavior when compared to Date and DateTime's strptime methods.

For example:

puts Date.strptime('201418', "%Y%U")
=> 2014-05-04
puts DateTime.strptime('201418', "%Y%U")
=> 2014-05-04T00:00:00+00:00
puts Time.strptime('201418', "%Y%U")
=> 2014-01-01 00:00:00 -0500

Files

time-strptime.patch (2.05 KB) time-strptime.patch jeremyevans0 (Jeremy Evans), 08/11/2019 07:32 PM

Related issues

Related to Ruby master - Bug #14241: Time.strptime() doesn't support the directive "%W".AssignedActions

History

Updated by matz (Yukihiro Matsumoto) over 5 years ago

  • Status changed from Open to Feedback

I am not sure what behavior you are referring as "extremely naive".
Please be concrete. How do you think it should behave?

Matz.

Updated by nobu (Nobuyoshi Nakada) over 5 years ago

I guess the OP expects %U not to be ignored.

diff --git a/lib/time.rb b/lib/time.rb
index 3728fef..ec45767 100644
--- a/lib/time.rb
+++ b/lib/time.rb
@@ -194,13 +194,18 @@ class Time

     LeapYearMonthDays = [31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31] # :nodoc:
     CommonYearMonthDays = [31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31] # :nodoc:
-    def month_days(y, m)
+    def month_days_of_year(y)
       if ((y % 4 == 0) && (y % 100 != 0)) || (y % 400 == 0)
-        LeapYearMonthDays[m-1]
+        LeapYearMonthDays
       else
-        CommonYearMonthDays[m-1]
+        CommonYearMonthDays
       end
     end
+    private :month_days_of_year
+
+    def month_days(y, m)
+      month_days_of_year(y)[m-1]
+    end
     private :month_days

     def apply_offset(year, mon, day, hour, min, sec, off)
@@ -432,7 +437,34 @@ class Time
       else
         year = d[:year]
         year = yield(year) if year && block_given?
-        t = make_time(date, year, d[:mon], d[:mday], d[:hour], d[:min], d[:sec], d[:sec_fraction], d[:zone], now)
+        mon = d[:mon]
+        mday = d[:mday]
+        unless mon and mday
+          case
+          when (w = d[:wnum0])  # %U
+            range = 0..53
+            mday = +1
+          when (w = d[:wnum1])  # %W
+            range = 0..53
+            mday = +2
+          when (w = d[:cweek])  # %V
+            range = 1..53
+            mday = -6
+          end
+          if mday
+            raise ArgumentError, "invalid date" unless range.cover?(w)
+            t = make_time(date, year, 1, 1, nil, nil, nil, nil, d[:zone], now)
+            mday += w * 7 - t.wday
+            if w = d[:wday] || d[:cwday]
+              mday += w
+            end
+            month_days_of_year(t.year).each_with_index do |n, m|
+              break mon = m+1 if mday <= n
+              mday -= n
+            end
+          end
+        end
+        t = make_time(date, year, mon, mday, d[:hour], d[:min], d[:sec], d[:sec_fraction], d[:zone], now)
       end
       t
     end

Updated by silverhammermba (Max Anselm) over 5 years ago

Sorry for the unclear wording. I would expect that it behaves similar to DateTime's implementation and to not ignore any fields returned by Date._strptime. At the very least the documentation should reflect the fact that it will only use certain values when constructing the Time object.

The fact that

Date.strptime("201418", "%Y%U").to_time
Time.strptime("201418", "%Y%U")

return different times is very surprising.

#4

Updated by jeremyevans0 (Jeremy Evans) 2 months ago

  • Related to Bug #14241: Time.strptime() doesn't support the directive "%W". added

Updated by jeremyevans0 (Jeremy Evans) 2 months ago

I tried updating the patch to apply to the master branch (it is attached). It doesn't appear to handle %W correctly:

str = Time.local(2019, 1, 30).strftime('%w %W %Y')
# => "3 04 2019"

Date.strptime(str, '%w %W %Y')
# => #<Date: 2019-01-30 ((2458514j,0s,0n),+0s,2299161j)>

Time.strptime(str, '%w %W %Y')
# => 2019-01-31 00:00:00 -0800

I think the approach I'm proposing in #14241 is simpler, by leaving the calculation to Date.strptime.

Also available in: Atom PDF