Project

General

Profile

Feature #9549

Improvements to Time::strptime

Added by sferik (Erik Michaels-Ober) almost 6 years ago. Updated about 2 years ago.

Status:
Rejected
Priority:
Normal
Target version:
[ruby-core:60954]

Description

I opened a pull request on GitHub a few days ago but wanted to open a parallel issue here, since I think this is the preferred tracker.

This patch includes three commits:

  • Add default arguments to Time::strptime to match Date::strptime. After this patch, Time::strptime may be invoked with 0, 1, or 2 arguments. When invoked with 0 or 1, it produces a result equivalent to Date::strptime invoked with the same arguments.
  • Raise ArgumentError if time passed to Time::strptime is invalid. This fixes a Ruby bug and adds a test to ensure it will not regress. Before this commit:

    require 'date'
    Date::strptime('31/02/2014', '%d/%m/%Y') # ArgumentError: invalid date
    require 'time'
    Time::strptime('31/02/2014', '%d/%m/%Y') # 2014-03-03 00:00:00 +0000

  • I have also renamed variables in Time::parse to be consistent with the changes I made to Time::strptime. Specifically, renaming d to hash, since it is not a Date and renaming the date parameter to time. I believe both of these changes make the code clearer.

Thank you for considering this patch.


Files

540.patch (4.71 KB) 540.patch sferik (Erik Michaels-Ober), 02/21/2014 09:05 PM

Related issues

Related to Ruby master - Bug #10588: Invalid DatesRejected12/11/2014Actions

History

Updated by nagachika (Tomoyuki Chikanaga) almost 6 years ago

  • Status changed from Open to Assigned
  • Assignee set to akr (Akira Tanaka)

Updated by akr (Akira Tanaka) over 5 years ago

  • Status changed from Assigned to Feedback

Erik Michaels-Ober wrote:

  • Add default arguments to Time::strptime to match Date::strptime. After this patch, Time::strptime may be invoked with 0, 1, or 2 arguments. When invoked with 0 or 1, it produces a result equivalent to Date::strptime invoked with the same arguments.

I'm not sure that is a good idea.
For example, -4712-01-01 is not a special date in Time.

  • Raise ArgumentError if time passed to Time::strptime is invalid. This fixes a Ruby bug and adds a test to ensure it will not regress. Before this commit:

    require 'date'
    Date::strptime('31/02/2014', '%d/%m/%Y') # ArgumentError: invalid date
    require 'time'
    Time::strptime('31/02/2014', '%d/%m/%Y') # 2014-03-03 00:00:00 +0000

I'm reluctant to such a restriction.

I tried to verify invalid time using round trip test once.
(ruby-core:14517, ruby-dev:33058, r14765, r15203)

But it caused bigger problems over benefits.

Updated by akr (Akira Tanaka) almost 5 years ago

#4

Updated by akr (Akira Tanaka) about 2 years ago

  • Status changed from Feedback to Rejected

Also available in: Atom PDF