Project

General

Profile

Actions

Feature #9549

closed

Improvements to Time::strptime

Added by sferik (Erik Michaels-Ober) over 10 years ago. Updated about 7 years ago.

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

Related to Ruby master - Bug #10588: Invalid DatesRejectedakr (Akira Tanaka)12/11/2014Actions

Updated by nagachika (Tomoyuki Chikanaga) over 10 years ago

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

Updated by akr (Akira Tanaka) over 10 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 10 years ago

Actions #4

Updated by akr (Akira Tanaka) about 7 years ago

  • Status changed from Feedback to Rejected
Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0