Project

General

Profile

Actions

Feature #18033

open

Time.new to parse a string

Added by nobu (Nobuyoshi Nakada) 2 months ago. Updated 2 months ago.

Status:
Open
Priority:
Normal
Assignee:
-
Target version:
-
[ruby-core:104552]

Description

Make Time.new parse Time#inspect and ISO-8601 like strings.

  • Time.iso8601 and Time.parse need an extension library, date.
  • Time.iso8601 can't parse Time#inspect string.
  • Time.parse often results in unintentional/surprising results.
  • Time.new also about 1.9 times faster than Time.iso8601.

    $ ./ruby -rtime -rbenchmark -e '
    n = 1000
    s = Time.now.iso8601
    Benchmark.bm(12) do |x|
      x.report("Time.iso8601") {n.times{Time.iso8601(s)}}
      x.report("Time.parse") {n.times{Time.parse(s)}}
      x.report("Time.new") {n.times{Time.new(s)}}
    end'
                       user     system      total        real
    Time.iso8601   0.006919   0.000185   0.007104 (  0.007091)
    Time.parse     0.018338   0.000207   0.018545 (  0.018590)
    Time.new       0.003671   0.000069   0.003740 (  0.003741)
    

https://github.com/ruby/ruby/pull/4639


Related issues

Related to Ruby master - Feature #16005: A variation of Time.iso8601 that can parse yyyy-MM-dd HH:mm:ssOpenActions
Actions #1

Updated by nobu (Nobuyoshi Nakada) 2 months ago

  • Description updated (diff)

Updated by ioquatix (Samuel Williams) 2 months ago

It looks like good improvement.

But does any other #new implementation work this way? Can we fix Time#parse? I wonder if user will be confused, should they use Time.parse or Time.new?

Why is Time.parse so slow?

This seems like a good performance improvement. But rather than fixing the existing #parse, we introduce new one. Now we move the complexity to the user.

Updated by nobu (Nobuyoshi Nakada) 2 months ago

ioquatix (Samuel Williams) wrote in #note-2:

But does any other #new implementation work this way? Can we fix Time#parse? I wonder if user will be confused, should they use Time.parse or Time.new?

The reason not to "fix Time#parse" is

Time.parse often results in unintentional/surprising results.

Updated by ioquatix (Samuel Williams) 2 months ago

Time.parse often results in unintentional/surprising results.

Can we change it so that it doesn't return unintentional/surprising results? I realise this might be impossible without extending the interface... e.g. Time.parse(..., surprising: false) :p

I agree clean room implementation is nice. But if Time.parse can do unexpected things, a new interface does not fix existing usage so the problem still exists, and now we have two interfaces for doing largely the same thing - one that "works" and one that does "unintentional/surprising" things.

Updated by nobu (Nobuyoshi Nakada) 2 months ago

ioquatix (Samuel Williams) wrote in #note-4:

Time.parse often results in unintentional/surprising results.

Can we change it so that it doesn't return unintentional/surprising results? I realise this might be impossible without extending the interface... e.g. Time.parse(..., surprising: false) :p

Yes, it seems by design, and changing the behavior will just break something.
I thought that the same name but different behavior method would be more confusing.

I agree clean room implementation is nice. But if Time.parse can do unexpected things, a new interface does not fix existing usage so the problem still exists, and now we have two interfaces for doing largely the same thing - one that "works" and one that does "unintentional/surprising" things.

A wrapper version based on the new Time.new would be possible, I think.

Updated by byroot (Jean Boussier) 2 months ago

                   user     system      total        real
Time.iso8601   0.006919   0.000185   0.007104 (  0.007091)
Time.parse     0.018338   0.000207   0.018545 (  0.018590)
Time.new       0.003671   0.000069   0.003740 (  0.003741)

This new interface being faster than Time.iso8601 at parsing iso8601 seems weird to me. When I have IS0-8601 dates (pretty much the only format I really use), I'd expect that by using the dedicated method to parse them, I get the best possible performance.

Can we optimize Time.iso8601 in the same way? Otherwise I can already see people moving to the new API and losing strictness by doing so.

Actions #7

Updated by mame (Yusuke Endoh) 2 months ago

  • Related to Feature #16005: A variation of Time.iso8601 that can parse yyyy-MM-dd HH:mm:ss added

Updated by ioquatix (Samuel Williams) 2 months ago

Yes, it seems by design, and changing the behavior will just break something.

I think there is value in the following interface:

Time.parse(..., strict: true) where we have some very clearly documented interpretation of strict.

If we want to support multiple formatS:

Time.parse(..., format: :loose/:strict) or Time.parse(...,format: :iso8601) etc.

Actions

Also available in: Atom PDF