From: akr@...
Date: 2014-05-05T03:41:58+00:00
Subject: [ruby-core:62375] [ruby-trunk - Feature #9549] [Feedback] Improvements to Time::strptime

Issue #9549 has been updated by Akira Tanaka.

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.


----------------------------------------
Feature #9549: Improvements to Time::strptime
https://bugs.ruby-lang.org/issues/9549#change-46534

* Author: Erik Michaels-Ober
* Status: Feedback
* Priority: Normal
* Assignee: Akira Tanaka
* Category: lib
* Target version: current: 2.2.0
----------------------------------------
I opened [a pull request on GitHub](https://github.com/ruby/ruby/pull/540) 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)


-- 
https://bugs.ruby-lang.org/