ruby-core@ruby-lang.org archive (unofficial mirror)
 help / color / mirror / Atom feed
* [ruby-core:104552] [Ruby master Feature#18033] Time.new to parse a string
@ 2021-07-09  6:13 nobu
  2021-07-09  6:30 ` [ruby-core:104554] " samuel
                   ` (24 more replies)
  0 siblings, 25 replies; 26+ messages in thread
From: nobu @ 2021-07-09  6:13 UTC (permalink / raw)
  To: ruby-core

Issue #18033 has been reported by nobu (Nobuyoshi Nakada).

----------------------------------------
Feature #18033: Time.new to parse a string
https://bugs.ruby-lang.org/issues/18033

* Author: nobu (Nobuyoshi Nakada)
* Status: Open
* Priority: Normal
----------------------------------------
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://bugs.ruby-lang.org/

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [ruby-core:104554] [Ruby master Feature#18033] Time.new to parse a string
  2021-07-09  6:13 [ruby-core:104552] [Ruby master Feature#18033] Time.new to parse a string nobu
@ 2021-07-09  6:30 ` samuel
  2021-07-09  7:51 ` [ruby-core:104559] " nobu
                   ` (23 subsequent siblings)
  24 siblings, 0 replies; 26+ messages in thread
From: samuel @ 2021-07-09  6:30 UTC (permalink / raw)
  To: ruby-core

Issue #18033 has been updated by ioquatix (Samuel Williams).


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.

----------------------------------------
Feature #18033: Time.new to parse a string
https://bugs.ruby-lang.org/issues/18033#change-92833

* Author: nobu (Nobuyoshi Nakada)
* Status: Open
* Priority: Normal
----------------------------------------
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



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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [ruby-core:104559] [Ruby master Feature#18033] Time.new to parse a string
  2021-07-09  6:13 [ruby-core:104552] [Ruby master Feature#18033] Time.new to parse a string nobu
  2021-07-09  6:30 ` [ruby-core:104554] " samuel
@ 2021-07-09  7:51 ` nobu
  2021-07-09  8:29 ` [ruby-core:104562] " samuel
                   ` (22 subsequent siblings)
  24 siblings, 0 replies; 26+ messages in thread
From: nobu @ 2021-07-09  7:51 UTC (permalink / raw)
  To: ruby-core

Issue #18033 has been updated by nobu (Nobuyoshi Nakada).


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.


----------------------------------------
Feature #18033: Time.new to parse a string
https://bugs.ruby-lang.org/issues/18033#change-92840

* Author: nobu (Nobuyoshi Nakada)
* Status: Open
* Priority: Normal
----------------------------------------
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



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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [ruby-core:104562] [Ruby master Feature#18033] Time.new to parse a string
  2021-07-09  6:13 [ruby-core:104552] [Ruby master Feature#18033] Time.new to parse a string nobu
  2021-07-09  6:30 ` [ruby-core:104554] " samuel
  2021-07-09  7:51 ` [ruby-core:104559] " nobu
@ 2021-07-09  8:29 ` samuel
  2021-07-09  8:51 ` [ruby-core:104564] " nobu
                   ` (21 subsequent siblings)
  24 siblings, 0 replies; 26+ messages in thread
From: samuel @ 2021-07-09  8:29 UTC (permalink / raw)
  To: ruby-core

Issue #18033 has been updated by ioquatix (Samuel Williams).


> Time.parse often results in unintentional/surprising results.

Can we change it so that it doesn't return unintentional/surprising results?

I agree clean room implementation is nice. But it `Time.parse` can do unexpected things, a new interface does not fix existing usage so the problem still exists.

----------------------------------------
Feature #18033: Time.new to parse a string
https://bugs.ruby-lang.org/issues/18033#change-92842

* Author: nobu (Nobuyoshi Nakada)
* Status: Open
* Priority: Normal
----------------------------------------
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



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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [ruby-core:104564] [Ruby master Feature#18033] Time.new to parse a string
  2021-07-09  6:13 [ruby-core:104552] [Ruby master Feature#18033] Time.new to parse a string nobu
                   ` (2 preceding siblings ...)
  2021-07-09  8:29 ` [ruby-core:104562] " samuel
@ 2021-07-09  8:51 ` nobu
  2021-07-10  8:42 ` [ruby-core:104575] " jean.boussier
                   ` (20 subsequent siblings)
  24 siblings, 0 replies; 26+ messages in thread
From: nobu @ 2021-07-09  8:51 UTC (permalink / raw)
  To: ruby-core

Issue #18033 has been updated by nobu (Nobuyoshi Nakada).


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.

----------------------------------------
Feature #18033: Time.new to parse a string
https://bugs.ruby-lang.org/issues/18033#change-92844

* Author: nobu (Nobuyoshi Nakada)
* Status: Open
* Priority: Normal
----------------------------------------
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



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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [ruby-core:104575] [Ruby master Feature#18033] Time.new to parse a string
  2021-07-09  6:13 [ruby-core:104552] [Ruby master Feature#18033] Time.new to parse a string nobu
                   ` (3 preceding siblings ...)
  2021-07-09  8:51 ` [ruby-core:104564] " nobu
@ 2021-07-10  8:42 ` jean.boussier
  2021-07-16  2:48 ` [ruby-core:104626] " samuel
                   ` (19 subsequent siblings)
  24 siblings, 0 replies; 26+ messages in thread
From: jean.boussier @ 2021-07-10  8:42 UTC (permalink / raw)
  To: ruby-core

Issue #18033 has been updated by byroot (Jean Boussier).


```
                   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.

----------------------------------------
Feature #18033: Time.new to parse a string
https://bugs.ruby-lang.org/issues/18033#change-92857

* Author: nobu (Nobuyoshi Nakada)
* Status: Open
* Priority: Normal
----------------------------------------
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



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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [ruby-core:104626] [Ruby master Feature#18033] Time.new to parse a string
  2021-07-09  6:13 [ruby-core:104552] [Ruby master Feature#18033] Time.new to parse a string nobu
                   ` (4 preceding siblings ...)
  2021-07-10  8:42 ` [ruby-core:104575] " jean.boussier
@ 2021-07-16  2:48 ` samuel
  2021-10-24 23:14 ` [ruby-core:105763] " ioquatix (Samuel Williams)
                   ` (18 subsequent siblings)
  24 siblings, 0 replies; 26+ messages in thread
From: samuel @ 2021-07-16  2:48 UTC (permalink / raw)
  To: ruby-core

Issue #18033 has been updated by ioquatix (Samuel Williams).


> 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`.

----------------------------------------
Feature #18033: Time.new to parse a string
https://bugs.ruby-lang.org/issues/18033#change-92921

* Author: nobu (Nobuyoshi Nakada)
* Status: Open
* Priority: Normal
----------------------------------------
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



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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [ruby-core:105763] [Ruby master Feature#18033] Time.new to parse a string
  2021-07-09  6:13 [ruby-core:104552] [Ruby master Feature#18033] Time.new to parse a string nobu
                   ` (5 preceding siblings ...)
  2021-07-16  2:48 ` [ruby-core:104626] " samuel
@ 2021-10-24 23:14 ` ioquatix (Samuel Williams)
  2021-11-10 10:44 ` [ruby-core:106002] " Eregon (Benoit Daloze)
                   ` (17 subsequent siblings)
  24 siblings, 0 replies; 26+ messages in thread
From: ioquatix (Samuel Williams) @ 2021-10-24 23:14 UTC (permalink / raw)
  To: ruby-core

Issue #18033 has been updated by ioquatix (Samuel Williams).


I checked the latest update.

I'm still not sure about `Time.new(string)` parsing a string.

What about `Time.parse_strict` or `Time.parse_iso8601` etc?

----------------------------------------
Feature #18033: Time.new to parse a string
https://bugs.ruby-lang.org/issues/18033#change-94277

* Author: nobu (Nobuyoshi Nakada)
* Status: Open
* Priority: Normal
----------------------------------------
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



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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [ruby-core:106002] [Ruby master Feature#18033] Time.new to parse a string
  2021-07-09  6:13 [ruby-core:104552] [Ruby master Feature#18033] Time.new to parse a string nobu
                   ` (6 preceding siblings ...)
  2021-10-24 23:14 ` [ruby-core:105763] " ioquatix (Samuel Williams)
@ 2021-11-10 10:44 ` Eregon (Benoit Daloze)
  2021-11-10 11:26 ` [ruby-core:106007] " byroot (Jean Boussier)
                   ` (16 subsequent siblings)
  24 siblings, 0 replies; 26+ messages in thread
From: Eregon (Benoit Daloze) @ 2021-11-10 10:44 UTC (permalink / raw)
  To: ruby-core

Issue #18033 has been updated by Eregon (Benoit Daloze).


Like @ioquatix and @byroot I think `Time.new` taking a iso-like String is not a good idea.

Also it's technically incompatible (the first argument is always the year for `Time.new` and it even accepts strings):
```
> Time.new "1234-02-03T00:00:00+00:34"
=> 1234-01-01 00:00:00 +0034
```

I understand `Time.iso8601 and Time.parse need an extension library, date.` is annoying and not intuitive.
How about simply making `Time.iso8601` a core method?

> Time.iso8601 can't parse Time#inspect string.

One shouldn't parse `inspect` so this should be no problem in practice.
We could add `Time#iso8601` though (in core), I think that's much better than `time.to_datetime.iso8601`.

----------------------------------------
Feature #18033: Time.new to parse a string
https://bugs.ruby-lang.org/issues/18033#change-94550

* Author: nobu (Nobuyoshi Nakada)
* Status: Open
* Priority: Normal
----------------------------------------
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



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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [ruby-core:106007] [Ruby master Feature#18033] Time.new to parse a string
  2021-07-09  6:13 [ruby-core:104552] [Ruby master Feature#18033] Time.new to parse a string nobu
                   ` (7 preceding siblings ...)
  2021-11-10 10:44 ` [ruby-core:106002] " Eregon (Benoit Daloze)
@ 2021-11-10 11:26 ` byroot (Jean Boussier)
  2021-11-12 11:33 ` [ruby-core:106036] " timcraft (Tim Craft)
                   ` (15 subsequent siblings)
  24 siblings, 0 replies; 26+ messages in thread
From: byroot (Jean Boussier) @ 2021-11-10 11:26 UTC (permalink / raw)
  To: ruby-core

Issue #18033 has been updated by byroot (Jean Boussier).


If I might add a nitpick, the actual format is RFC 3339, which is pretty much a subset of ISO 8601.

But yes, +1 to making `Time.iso8601` a core method.

----------------------------------------
Feature #18033: Time.new to parse a string
https://bugs.ruby-lang.org/issues/18033#change-94556

* Author: nobu (Nobuyoshi Nakada)
* Status: Open
* Priority: Normal
----------------------------------------
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



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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [ruby-core:106036] [Ruby master Feature#18033] Time.new to parse a string
  2021-07-09  6:13 [ruby-core:104552] [Ruby master Feature#18033] Time.new to parse a string nobu
                   ` (8 preceding siblings ...)
  2021-11-10 11:26 ` [ruby-core:106007] " byroot (Jean Boussier)
@ 2021-11-12 11:33 ` timcraft (Tim Craft)
  2021-12-03  4:07 ` [ruby-core:106440] " nobu (Nobuyoshi Nakada)
                   ` (14 subsequent siblings)
  24 siblings, 0 replies; 26+ messages in thread
From: timcraft (Tim Craft) @ 2021-11-12 11:33 UTC (permalink / raw)
  To: ruby-core

Issue #18033 has been updated by timcraft (Tim Craft).


Nice performance improvement!

In terms of the interface I think it would be confusing to make `Time.new` parse a string:
* Using a `.parse` method for parsing is more conventional / less surprising / more intention revealing
* Having *both* `.new` and `.parse` would be confusing because it's not clear when to use one or the other, what the trade-offs are etc

It feels very unintiutive and not "least surprise". Similarly `Time.parse(...,format: :iso8601)` and `Time.parse_iso8601` feel a bit clunky.

Is this awkwardness stemming from trying to fit much functionality into the Time class? It would be more work, but if there was a dedicated ISO8601 module we could shift some responsibility away from the Time class. For example:

```
ISO8601::Time.parse  # strictly ISO8601 parsing, returns a Time object
```

This could be extended to encapsulate ISO8601 parsing for other classes:

```
ISO8601::Date.parse
ISO8601::Duration.parse
ISO8601::TimeInterval.parse
```

The same general pattern could be followed for other formats:

```
RFC3339::Time.parse
RFC2822::Time.parse
SQL92::Time.parse
```

Each of those methods would have the same signature, so they could be easily interchanged to get varying degrees of strictness or performance.

From a naming and readability perspective the names are less surprising, more intention revealing, and very greppable.

The higher level `Time.parse` method could potentially delegate to these methods to support parsing a wider range of formats.

Each module could also be extended to encapsulate string formatting as an alternative to adding instance methods like #iso8601, #rfc3339 etc.


***

An alternative to using the formats as the top level modules would be to invert and nest the modules under the Time classes, for example:

```
Time::ISO8601.parse  # instead of ISO8601::Time.parse
```

Same benefits—I'm not sure which would be considered more Ruby-ish?

----------------------------------------
Feature #18033: Time.new to parse a string
https://bugs.ruby-lang.org/issues/18033#change-94627

* Author: nobu (Nobuyoshi Nakada)
* Status: Open
* Priority: Normal
----------------------------------------
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



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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [ruby-core:106440] [Ruby master Feature#18033] Time.new to parse a string
  2021-07-09  6:13 [ruby-core:104552] [Ruby master Feature#18033] Time.new to parse a string nobu
                   ` (9 preceding siblings ...)
  2021-11-12 11:33 ` [ruby-core:106036] " timcraft (Tim Craft)
@ 2021-12-03  4:07 ` nobu (Nobuyoshi Nakada)
  2021-12-03  6:54 ` [ruby-core:106443] " nobu (Nobuyoshi Nakada)
                   ` (13 subsequent siblings)
  24 siblings, 0 replies; 26+ messages in thread
From: nobu (Nobuyoshi Nakada) @ 2021-12-03  4:07 UTC (permalink / raw)
  To: ruby-core

Issue #18033 has been updated by nobu (Nobuyoshi Nakada).


Eregon (Benoit Daloze) wrote in #note-10:
> Also it's technically incompatible (the first argument is always the year for `Time.new` and it even accepts strings):
> ```
> > Time.new "2234-01-01T00:00:00+07:00"
> => 2234-01-01 00:00:00 +0100
> ```

I overlooked it.
May #18331 be better?


----------------------------------------
Feature #18033: Time.new to parse a string
https://bugs.ruby-lang.org/issues/18033#change-95085

* Author: nobu (Nobuyoshi Nakada)
* Status: Open
* Priority: Normal
----------------------------------------
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



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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [ruby-core:106443] [Ruby master Feature#18033] Time.new to parse a string
  2021-07-09  6:13 [ruby-core:104552] [Ruby master Feature#18033] Time.new to parse a string nobu
                   ` (10 preceding siblings ...)
  2021-12-03  4:07 ` [ruby-core:106440] " nobu (Nobuyoshi Nakada)
@ 2021-12-03  6:54 ` nobu (Nobuyoshi Nakada)
  2021-12-03 10:08 ` [ruby-core:106449] " sawa (Tsuyoshi Sawada)
                   ` (12 subsequent siblings)
  24 siblings, 0 replies; 26+ messages in thread
From: nobu (Nobuyoshi Nakada) @ 2021-12-03  6:54 UTC (permalink / raw)
  To: ruby-core

Issue #18033 has been updated by nobu (Nobuyoshi Nakada).


I remember #18331 won't work with subclasses of `Time`.
Does `Time.at("2021-12-04 02:00:00")` make sense?

----------------------------------------
Feature #18033: Time.new to parse a string
https://bugs.ruby-lang.org/issues/18033#change-95091

* Author: nobu (Nobuyoshi Nakada)
* Status: Open
* Priority: Normal
----------------------------------------
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



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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [ruby-core:106449] [Ruby master Feature#18033] Time.new to parse a string
  2021-07-09  6:13 [ruby-core:104552] [Ruby master Feature#18033] Time.new to parse a string nobu
                   ` (11 preceding siblings ...)
  2021-12-03  6:54 ` [ruby-core:106443] " nobu (Nobuyoshi Nakada)
@ 2021-12-03 10:08 ` sawa (Tsuyoshi Sawada)
  2021-12-03 10:23 ` [ruby-core:106451] " sawa (Tsuyoshi Sawada)
                   ` (11 subsequent siblings)
  24 siblings, 0 replies; 26+ messages in thread
From: sawa (Tsuyoshi Sawada) @ 2021-12-03 10:08 UTC (permalink / raw)
  To: ruby-core

Issue #18033 has been updated by sawa (Tsuyoshi Sawada).


nobu (Nobuyoshi Nakada) wrote in #note-15:
> I remember #18331 won't work with subclasses of `Time`.

Can you elaborate on that? What problem does it have? Even more helpful if you comment on #18331.


----------------------------------------
Feature #18033: Time.new to parse a string
https://bugs.ruby-lang.org/issues/18033#change-95104

* Author: nobu (Nobuyoshi Nakada)
* Status: Open
* Priority: Normal
----------------------------------------
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



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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [ruby-core:106451] [Ruby master Feature#18033] Time.new to parse a string
  2021-07-09  6:13 [ruby-core:104552] [Ruby master Feature#18033] Time.new to parse a string nobu
                   ` (12 preceding siblings ...)
  2021-12-03 10:08 ` [ruby-core:106449] " sawa (Tsuyoshi Sawada)
@ 2021-12-03 10:23 ` sawa (Tsuyoshi Sawada)
  2021-12-03 11:33 ` [ruby-core:106452] " Eregon (Benoit Daloze)
                   ` (10 subsequent siblings)
  24 siblings, 0 replies; 26+ messages in thread
From: sawa (Tsuyoshi Sawada) @ 2021-12-03 10:23 UTC (permalink / raw)
  To: ruby-core

Issue #18033 has been updated by sawa (Tsuyoshi Sawada).


nobu (Nobuyoshi Nakada) wrote in #note-15:
> I remember #18331 won't work with subclasses of `Time`.

Can you elaborate on that? Is your concern about certain frameworks using time-like classes other than `Time`? I don't see any problem with that. If you create a `Time` object using `Kernel.#Time` proposed in #18331, that can easily be converted (often without noticing) to such class in case of Ruby on Rails' `ActiveSupport::TimeWithZone`.

----------------------------------------
Feature #18033: Time.new to parse a string
https://bugs.ruby-lang.org/issues/18033#change-95106

* Author: nobu (Nobuyoshi Nakada)
* Status: Open
* Priority: Normal
----------------------------------------
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



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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [ruby-core:106452] [Ruby master Feature#18033] Time.new to parse a string
  2021-07-09  6:13 [ruby-core:104552] [Ruby master Feature#18033] Time.new to parse a string nobu
                   ` (13 preceding siblings ...)
  2021-12-03 10:23 ` [ruby-core:106451] " sawa (Tsuyoshi Sawada)
@ 2021-12-03 11:33 ` Eregon (Benoit Daloze)
  2021-12-03 13:21 ` [ruby-core:106456] " nobu (Nobuyoshi Nakada)
                   ` (9 subsequent siblings)
  24 siblings, 0 replies; 26+ messages in thread
From: Eregon (Benoit Daloze) @ 2021-12-03 11:33 UTC (permalink / raw)
  To: ruby-core

Issue #18033 has been updated by Eregon (Benoit Daloze).


Yes, `Kernel#Time(str)` seems better than `Time.new(str)`.

However I think being precise for parsing is key for times.
Hence, why not make `Time.iso8601` fast and a core method?
Then code already using the right method would just become faster, and there is no need migrate code to use a new API.

----------------------------------------
Feature #18033: Time.new to parse a string
https://bugs.ruby-lang.org/issues/18033#change-95109

* Author: nobu (Nobuyoshi Nakada)
* Status: Open
* Priority: Normal
----------------------------------------
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



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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [ruby-core:106456] [Ruby master Feature#18033] Time.new to parse a string
  2021-07-09  6:13 [ruby-core:104552] [Ruby master Feature#18033] Time.new to parse a string nobu
                   ` (14 preceding siblings ...)
  2021-12-03 11:33 ` [ruby-core:106452] " Eregon (Benoit Daloze)
@ 2021-12-03 13:21 ` nobu (Nobuyoshi Nakada)
  2021-12-03 13:49 ` [ruby-core:106458] " nobu (Nobuyoshi Nakada)
                   ` (8 subsequent siblings)
  24 siblings, 0 replies; 26+ messages in thread
From: nobu (Nobuyoshi Nakada) @ 2021-12-03 13:21 UTC (permalink / raw)
  To: ruby-core

Issue #18033 has been updated by nobu (Nobuyoshi Nakada).


sawa (Tsuyoshi Sawada) wrote in #note-16:
> nobu (Nobuyoshi Nakada) wrote in #note-15:
> > I remember #18331 won't work with subclasses of `Time`.
> 
> Can you elaborate on that? Is your concern about certain frameworks using time-like classes other than `Time`? I don't see any problem with that. If you create a `Time` object using `Kernel.#Time` proposed in #18331, that can easily be converted (often without noticing) to such class in case of Ruby on Rails' `ActiveSupport::TimeWithZone`.

The timezone name will be converted by `find_timezone` method on the class of a `Time` instance, so that subclasses can override it.

----------------------------------------
Feature #18033: Time.new to parse a string
https://bugs.ruby-lang.org/issues/18033#change-95113

* Author: nobu (Nobuyoshi Nakada)
* Status: Open
* Priority: Normal
----------------------------------------
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



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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [ruby-core:106458] [Ruby master Feature#18033] Time.new to parse a string
  2021-07-09  6:13 [ruby-core:104552] [Ruby master Feature#18033] Time.new to parse a string nobu
                   ` (15 preceding siblings ...)
  2021-12-03 13:21 ` [ruby-core:106456] " nobu (Nobuyoshi Nakada)
@ 2021-12-03 13:49 ` nobu (Nobuyoshi Nakada)
  2021-12-06  4:55 ` [ruby-core:106501] " nobu (Nobuyoshi Nakada)
                   ` (7 subsequent siblings)
  24 siblings, 0 replies; 26+ messages in thread
From: nobu (Nobuyoshi Nakada) @ 2021-12-03 13:49 UTC (permalink / raw)
  To: ruby-core

Issue #18033 has been updated by nobu (Nobuyoshi Nakada).


Eregon (Benoit Daloze) wrote in #note-17:
> Hence, why not make `Time.iso8601` fast and a core method?
> Then code already using the right method would just become faster, and there is no need migrate code to use a new API.

First, the primary target is the result of `Time#inspect`, and it is not fully compliant with ISO-8601.
Second, ISO-8601 allows many variants, but I'm not going to implement them all.

----------------------------------------
Feature #18033: Time.new to parse a string
https://bugs.ruby-lang.org/issues/18033#change-95115

* Author: nobu (Nobuyoshi Nakada)
* Status: Open
* Priority: Normal
----------------------------------------
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



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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [ruby-core:106501] [Ruby master Feature#18033] Time.new to parse a string
  2021-07-09  6:13 [ruby-core:104552] [Ruby master Feature#18033] Time.new to parse a string nobu
                   ` (16 preceding siblings ...)
  2021-12-03 13:49 ` [ruby-core:106458] " nobu (Nobuyoshi Nakada)
@ 2021-12-06  4:55 ` nobu (Nobuyoshi Nakada)
  2021-12-06 13:18 ` [ruby-core:106512] " Eregon (Benoit Daloze)
                   ` (6 subsequent siblings)
  24 siblings, 0 replies; 26+ messages in thread
From: nobu (Nobuyoshi Nakada) @ 2021-12-06  4:55 UTC (permalink / raw)
  To: ruby-core

Issue #18033 has been updated by nobu (Nobuyoshi Nakada).


Eregon (Benoit Daloze) wrote in #note-10:
> Also it's technically incompatible (the first argument is always the year for `Time.new` and it even accepts strings):
> ```
> > Time.new "2234-01-01T00:00:00+07:00"
> => 2234-01-01 00:00:00 +0100
> ```

Actually this code raises an `ArgumentError` in the master, since https://bugs.ruby-lang.org/issues/17485#change-89871.

----------------------------------------
Feature #18033: Time.new to parse a string
https://bugs.ruby-lang.org/issues/18033#change-95161

* Author: nobu (Nobuyoshi Nakada)
* Status: Open
* Priority: Normal
----------------------------------------
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



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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [ruby-core:106512] [Ruby master Feature#18033] Time.new to parse a string
  2021-07-09  6:13 [ruby-core:104552] [Ruby master Feature#18033] Time.new to parse a string nobu
                   ` (17 preceding siblings ...)
  2021-12-06  4:55 ` [ruby-core:106501] " nobu (Nobuyoshi Nakada)
@ 2021-12-06 13:18 ` Eregon (Benoit Daloze)
  2021-12-06 14:54 ` [ruby-core:106516] " nobu (Nobuyoshi Nakada)
                   ` (5 subsequent siblings)
  24 siblings, 0 replies; 26+ messages in thread
From: Eregon (Benoit Daloze) @ 2021-12-06 13:18 UTC (permalink / raw)
  To: ruby-core

Issue #18033 has been updated by Eregon (Benoit Daloze).


nobu (Nobuyoshi Nakada) wrote in #note-19:
> First, the primary target is the result of `Time#inspect`, and it is not fully compliant with ISO-8601.

Thanks for making that clear, I wasn't sure why not just improve `Time.iso8601`.

Right, they differ:
```
irb(main):006:0> t.inspect
=> "2021-12-06 14:10:15.334531885 +0100"
irb(main):007:0> t.iso8601
=> "2021-12-06T14:10:15+01:00"
```

Why we do we need to parse `Time#inspect` output?
It seems in general bad to rely on `inspect` for serialization, since it only works for some classes.
Maybe to preserve subseconds?

`#inspect` does not feel like a proper way to serialize a Time instance, so if we want to add that I think we should have a new method for it too (maybe `#dump`?).

In any case, `Time.new` accepts individual time components, and I think making it parse strings is just confusing.
A new method `Time.try_convert` (or `Time.undump`) feels more appropriate for such parsing.

> Second, ISO-8601 allows many variants, but I'm not going to implement them all.

Is that what makes it faster?
The PR still uses a Regexp so I guess the main difference is the Regexp is a little bit simpler?
They look fairly similar:
* https://github.com/ruby/ruby/pull/4639/files
* https://github.com/ruby/ruby/blob/715a51a0d6963f9d727191d4e1ad0690fd28c4dd/lib/time.rb#L617-L650

----------------------------------------
Feature #18033: Time.new to parse a string
https://bugs.ruby-lang.org/issues/18033#change-95171

* Author: nobu (Nobuyoshi Nakada)
* Status: Open
* Priority: Normal
----------------------------------------
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



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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [ruby-core:106516] [Ruby master Feature#18033] Time.new to parse a string
  2021-07-09  6:13 [ruby-core:104552] [Ruby master Feature#18033] Time.new to parse a string nobu
                   ` (18 preceding siblings ...)
  2021-12-06 13:18 ` [ruby-core:106512] " Eregon (Benoit Daloze)
@ 2021-12-06 14:54 ` nobu (Nobuyoshi Nakada)
  2021-12-06 17:37 ` [ruby-core:106521] " Eregon (Benoit Daloze)
                   ` (4 subsequent siblings)
  24 siblings, 0 replies; 26+ messages in thread
From: nobu (Nobuyoshi Nakada) @ 2021-12-06 14:54 UTC (permalink / raw)
  To: ruby-core

Issue #18033 has been updated by nobu (Nobuyoshi Nakada).


Eregon (Benoit Daloze) wrote in #note-21:
> Why we do we need to parse `Time#inspect` output?
> It seems in general bad to rely on `inspect` for serialization, since it only works for some classes.
> Maybe to preserve subseconds?

I wrote `Time#inspect`, but the "ISO 8601-like" format is not only used by it, e.g., `--date=iso` of git.

> `#inspect` does not feel like a proper way to serialize a Time instance, so if we want to add that I think we should have a new method for it too (maybe `#dump`?).

> In any case, `Time.new` accepts individual time components, and I think making it parse strings is just confusing.

How about `Time.at`?

> A new method `Time.try_convert` (or `Time.undump`) feels more appropriate for such parsing.

`Time.try_convert` feels considerable, but passing the timezone option may not fit.

> > Second, ISO-8601 allows many variants, but I'm not going to implement them all.
> 
> Is that what makes it faster?

Not to be faster.
ISO-8601 is a large specification and allows many omissions, which makes parsing very complex and sometimes conflicts with the spec of `Time`.

> The PR still uses a Regexp so I guess the main difference is the Regexp is a little bit simpler?
> They look fairly similar:
> * https://github.com/ruby/ruby/pull/4639/files

That PR is abandoned, please see [Time.new-string] or [Time.at-string].

[Time.new-string]: https://github.com/ruby/ruby/pull/4825
[Time.at-string]: https://github.com/ruby/ruby/pull/5212

----------------------------------------
Feature #18033: Time.new to parse a string
https://bugs.ruby-lang.org/issues/18033#change-95177

* Author: nobu (Nobuyoshi Nakada)
* Status: Open
* Priority: Normal
----------------------------------------
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



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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [ruby-core:106521] [Ruby master Feature#18033] Time.new to parse a string
  2021-07-09  6:13 [ruby-core:104552] [Ruby master Feature#18033] Time.new to parse a string nobu
                   ` (19 preceding siblings ...)
  2021-12-06 14:54 ` [ruby-core:106516] " nobu (Nobuyoshi Nakada)
@ 2021-12-06 17:37 ` Eregon (Benoit Daloze)
  2021-12-07 14:15 ` [ruby-core:106537] " nobu (Nobuyoshi Nakada)
                   ` (3 subsequent siblings)
  24 siblings, 0 replies; 26+ messages in thread
From: Eregon (Benoit Daloze) @ 2021-12-06 17:37 UTC (permalink / raw)
  To: ruby-core

Issue #18033 has been updated by Eregon (Benoit Daloze).


nobu (Nobuyoshi Nakada) wrote in #note-22:
> I wrote `Time#inspect`, but the "ISO 8601-like" format is not only used by it, e.g., `--date=iso` of git.

Interesting that `git` has this too:
```
           --date=iso (or --date=iso8601) shows timestamps in a ISO 8601-like format. The differences to the strict ISO 8601 format are:
                        
           •   a space instead of the T date/time delimiter
           
           •   a space between time and time zone

           •   no colon between hours and minutes of the time zone

           --date=iso-strict (or --date=iso8601-strict) shows timestamps in strict ISO 8601 format.
```

Maybe we really need a name for this. Like `Time#iso` and `Time.iso`.

Or it could be a keyword argument: `time.iso8601(format: :git)` and `Time.iso8601(str, format: :git)`.
`:git` is just an example, could be `textual`, `relaxed` or anything.

Another idea would be to actually use the strict variant of iso8601 if that is well defined, like what `git log --date=iso-strict` shows.
Then we'd have `Time#iso8601_strict`/`Time.iso8601_strict`, or `time.iso8601(format: :strict)` and `Time.iso8601(str, format: :strict)`.
That feels proper to me because then we have a clear way to serialize and deserialize times and finding the dumping/parsing method is trivial (they have the same name, no need to guess).
I feel relying on `#inspect` for efficient serializing is in general bad, because that is not the main purpose of `#inspect` (rather it's to show a clear/debug-like representation of an object).

> How about `Time.at`?

To me `Time.at` sounds like "give me a Time at epoch", so I think it's unexpected it does String parsing.

> `Time.try_convert` feels considerable, but passing the timezone option may not fit.

I think an extra optional timezone argument would be fine.



----------------------------------------
Feature #18033: Time.new to parse a string
https://bugs.ruby-lang.org/issues/18033#change-95181

* Author: nobu (Nobuyoshi Nakada)
* Status: Open
* Priority: Normal
----------------------------------------
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



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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [ruby-core:106537] [Ruby master Feature#18033] Time.new to parse a string
  2021-07-09  6:13 [ruby-core:104552] [Ruby master Feature#18033] Time.new to parse a string nobu
                   ` (20 preceding siblings ...)
  2021-12-06 17:37 ` [ruby-core:106521] " Eregon (Benoit Daloze)
@ 2021-12-07 14:15 ` nobu (Nobuyoshi Nakada)
  2022-12-15  6:11 ` [ruby-core:111293] " mame (Yusuke Endoh)
                   ` (2 subsequent siblings)
  24 siblings, 0 replies; 26+ messages in thread
From: nobu (Nobuyoshi Nakada) @ 2021-12-07 14:15 UTC (permalink / raw)
  To: ruby-core

Issue #18033 has been updated by nobu (Nobuyoshi Nakada).


Eregon (Benoit Daloze) wrote in #note-23:
> > `Time.try_convert` feels considerable, but passing the timezone option may not fit.
> 
> I think an extra optional timezone argument would be fine.

Ah, my bad.
The `try_convert` methods are the implicit conversions for duck-typing, i.e, `to_int` to `Integer`, `to_str` to `String` and so on.
As parsing like `Kernel#Integer` is not an implicit conversion, `Time.try_convert` can't be valid.

----------------------------------------
Feature #18033: Time.new to parse a string
https://bugs.ruby-lang.org/issues/18033#change-95203

* Author: nobu (Nobuyoshi Nakada)
* Status: Open
* Priority: Normal
----------------------------------------
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



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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [ruby-core:111293] [Ruby master Feature#18033] Time.new to parse a string
  2021-07-09  6:13 [ruby-core:104552] [Ruby master Feature#18033] Time.new to parse a string nobu
                   ` (21 preceding siblings ...)
  2021-12-07 14:15 ` [ruby-core:106537] " nobu (Nobuyoshi Nakada)
@ 2022-12-15  6:11 ` mame (Yusuke Endoh)
  2022-12-15  6:54 ` [ruby-core:111295] " matz (Yukihiro Matsumoto)
  2024-10-11 12:49 ` [ruby-core:119511] " Eregon (Benoit Daloze) via ruby-core
  24 siblings, 0 replies; 26+ messages in thread
From: mame (Yusuke Endoh) @ 2022-12-15  6:11 UTC (permalink / raw)
  To: ruby-core

Issue #18033 has been updated by mame (Yusuke Endoh).


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

----------------------------------------
Feature #18033: Time.new to parse a string
https://bugs.ruby-lang.org/issues/18033#change-100651

* Author: nobu (Nobuyoshi Nakada)
* Status: Open
* Priority: Normal
----------------------------------------
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



-- 
https://bugs.ruby-lang.org/
 ______________________________________________
 ruby-core mailing list -- ruby-core@ml.ruby-lang.org
 To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org
 ruby-core info -- https://ml.ruby-lang.org/mailman3/postorius/lists/ruby-core.ml.ruby-lang.org/

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [ruby-core:111295] [Ruby master Feature#18033] Time.new to parse a string
  2021-07-09  6:13 [ruby-core:104552] [Ruby master Feature#18033] Time.new to parse a string nobu
                   ` (22 preceding siblings ...)
  2022-12-15  6:11 ` [ruby-core:111293] " mame (Yusuke Endoh)
@ 2022-12-15  6:54 ` matz (Yukihiro Matsumoto)
  2024-10-11 12:49 ` [ruby-core:119511] " Eregon (Benoit Daloze) via ruby-core
  24 siblings, 0 replies; 26+ messages in thread
From: matz (Yukihiro Matsumoto) @ 2022-12-15  6:54 UTC (permalink / raw)
  To: ruby-core

Issue #18033 has been updated by matz (Yukihiro Matsumoto).


Accepted.

Performance improvement is a great benefit.
Both `Time.iso8859` and `Time.parse` requires loading `date` gem, and providing those methods in the core may cause overwriting.
Currently, `Time.new` only takes numeric arguments, so adding extra argument pattern add only a small complexity.

Matz.


----------------------------------------
Feature #18033: Time.new to parse a string
https://bugs.ruby-lang.org/issues/18033#change-100654

* Author: nobu (Nobuyoshi Nakada)
* Status: Open
* Priority: Normal
----------------------------------------
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/4825




-- 
https://bugs.ruby-lang.org/
 ______________________________________________
 ruby-core mailing list -- ruby-core@ml.ruby-lang.org
 To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org
 ruby-core info -- https://ml.ruby-lang.org/mailman3/postorius/lists/ruby-core.ml.ruby-lang.org/

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [ruby-core:119511] [Ruby master Feature#18033] Time.new to parse a string
  2021-07-09  6:13 [ruby-core:104552] [Ruby master Feature#18033] Time.new to parse a string nobu
                   ` (23 preceding siblings ...)
  2022-12-15  6:54 ` [ruby-core:111295] " matz (Yukihiro Matsumoto)
@ 2024-10-11 12:49 ` Eregon (Benoit Daloze) via ruby-core
  24 siblings, 0 replies; 26+ messages in thread
From: Eregon (Benoit Daloze) via ruby-core @ 2024-10-11 12:49 UTC (permalink / raw)
  To: ruby-core; +Cc: Eregon (Benoit Daloze)

Issue #18033 has been updated by Eregon (Benoit Daloze).


matz (Yukihiro Matsumoto) wrote in #note-27:
> Currently, `Time.new` only takes numeric arguments, so adding extra argument pattern add only a small complexity.

But older versions like 3.0 accept year as a String: https://bugs.ruby-lang.org/issues/18033#note-10
So we get very weird behavior between versions:
```
$ ruby -ve 'p Time.new "2234-10-12T01:02:03"'
ruby 3.0.6p216 (2023-03-30 revision 23a532679b) [x86_64-linux]
2234-01-01 00:00:00 +0100

$ ruby -ve 'p Time.new "2234-10-12T01:02:03"'
ruby 3.1.3p185 (2022-11-24 revision 1a6b16756e) [x86_64-linux]
<internal:timev>:310:in `initialize': invalid value for Integer(): "2234-10-12T01:02:03" (ArgumentError)
	from -e:1:in `new'
	from -e:1:in `<main>'

$ ruby -ve 'p Time.new "2234-10-12T01:02:03"'
ruby 3.2.4 (2024-04-23 revision af471c0e01) [x86_64-linux]
2234-10-12 01:02:03 +0200

$ ruby -ve 'p Time.new "2234-10-12T01:02:03"'
ruby 3.3.5 (2024-09-03 revision ef084cc8f4) [x86_64-linux]
2234-10-12 01:02:03 +0200
```

----------------------------------------
Feature #18033: Time.new to parse a string
https://bugs.ruby-lang.org/issues/18033#change-110126

* Author: nobu (Nobuyoshi Nakada)
* Status: Closed
----------------------------------------
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/4825




-- 
https://bugs.ruby-lang.org/
 ______________________________________________
 ruby-core mailing list -- ruby-core@ml.ruby-lang.org
 To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org
 ruby-core info -- https://ml.ruby-lang.org/mailman3/lists/ruby-core.ml.ruby-lang.org/

^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2024-10-11 12:50 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-09  6:13 [ruby-core:104552] [Ruby master Feature#18033] Time.new to parse a string nobu
2021-07-09  6:30 ` [ruby-core:104554] " samuel
2021-07-09  7:51 ` [ruby-core:104559] " nobu
2021-07-09  8:29 ` [ruby-core:104562] " samuel
2021-07-09  8:51 ` [ruby-core:104564] " nobu
2021-07-10  8:42 ` [ruby-core:104575] " jean.boussier
2021-07-16  2:48 ` [ruby-core:104626] " samuel
2021-10-24 23:14 ` [ruby-core:105763] " ioquatix (Samuel Williams)
2021-11-10 10:44 ` [ruby-core:106002] " Eregon (Benoit Daloze)
2021-11-10 11:26 ` [ruby-core:106007] " byroot (Jean Boussier)
2021-11-12 11:33 ` [ruby-core:106036] " timcraft (Tim Craft)
2021-12-03  4:07 ` [ruby-core:106440] " nobu (Nobuyoshi Nakada)
2021-12-03  6:54 ` [ruby-core:106443] " nobu (Nobuyoshi Nakada)
2021-12-03 10:08 ` [ruby-core:106449] " sawa (Tsuyoshi Sawada)
2021-12-03 10:23 ` [ruby-core:106451] " sawa (Tsuyoshi Sawada)
2021-12-03 11:33 ` [ruby-core:106452] " Eregon (Benoit Daloze)
2021-12-03 13:21 ` [ruby-core:106456] " nobu (Nobuyoshi Nakada)
2021-12-03 13:49 ` [ruby-core:106458] " nobu (Nobuyoshi Nakada)
2021-12-06  4:55 ` [ruby-core:106501] " nobu (Nobuyoshi Nakada)
2021-12-06 13:18 ` [ruby-core:106512] " Eregon (Benoit Daloze)
2021-12-06 14:54 ` [ruby-core:106516] " nobu (Nobuyoshi Nakada)
2021-12-06 17:37 ` [ruby-core:106521] " Eregon (Benoit Daloze)
2021-12-07 14:15 ` [ruby-core:106537] " nobu (Nobuyoshi Nakada)
2022-12-15  6:11 ` [ruby-core:111293] " mame (Yusuke Endoh)
2022-12-15  6:54 ` [ruby-core:111295] " matz (Yukihiro Matsumoto)
2024-10-11 12:49 ` [ruby-core:119511] " Eregon (Benoit Daloze) via ruby-core

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).