Project

General

Profile

Bug #9836

Bad Implementation of Time.strptime

Added by silverhammermba (Max Anselm) almost 6 years ago. Updated 3 months ago.

Status:
Closed
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".Closedakr (Akira Tanaka)Actions

Updated by matz (Yukihiro Matsumoto) almost 6 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) almost 6 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) almost 6 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) 7 months ago

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

Updated by jeremyevans0 (Jeremy Evans) 7 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.

#6

Updated by jeremyevans (Jeremy Evans) 3 months ago

  • Status changed from Feedback to Closed

Applied in changeset git|a9d4f2d03c847ec1c89dc03a5076a9fa29ffa61f.


Support %U/%u/%W/%w/%V/%g/%G formats in Time.strptime

Most of these formats were documented as supported, but were not
actually supported. Document that %g and %G are supported.

If %U/%W is specified without yday and mon/mday are not specified,
then Date.strptime is used to get the appropriate yday.

If cwyear is specifier without the year, or cwday and cweek are
specified without mday and mon, then use Date.strptime and convert
the resulting value to Time, since Time.make_time cannot handle
those conversions

Fixes [Bug #9836]
Fixes [Bug #14241]

Also available in: Atom PDF