Project

General

Profile

Actions

Bug #9836

closed

Bad Implementation of Time.strptime

Added by silverhammermba (Max Anselm) over 10 years ago. Updated almost 5 years ago.

Status:
Closed
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 1 (0 open1 closed)

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

Updated by matz (Yukihiro Matsumoto) over 10 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 10 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 10 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.

Actions #4

Updated by jeremyevans0 (Jeremy Evans) about 5 years ago

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

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

Actions #6

Updated by jeremyevans (Jeremy Evans) almost 5 years 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]

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0