New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DateTime.iso8601 fails with an error if a second fraction is present #2941

Closed
wants to merge 1 commit into
base: jruby-1_7
from

Conversation

Projects
None yet
4 participants
@azolotko
Contributor

azolotko commented May 14, 2015

Fixes #2883

I would be happy to add a test, if I only knew where to put it.

Thanks!

@azolotko azolotko closed this May 16, 2015

@azolotko azolotko reopened this May 16, 2015

@BanzaiMan

This comment has been minimized.

Show comment
Hide comment
@BanzaiMan

BanzaiMan May 16, 2015

Member

Keep in mind that lib/ruby/1.9 is where MRI standard libraries live. Things should not diverge there.

Member

BanzaiMan commented May 16, 2015

Keep in mind that lib/ruby/1.9 is where MRI standard libraries live. Things should not diverge there.

@azolotko

This comment has been minimized.

Show comment
Hide comment
@azolotko

azolotko May 17, 2015

Contributor

@BanzaiMan Yes, absolutely. Please correct me if I'm wrong, but MRI seems to do a similar thing: https://github.com/ruby/ruby/blob/trunk/ext/date/date_parse.c#L2363

Contributor

azolotko commented May 17, 2015

@BanzaiMan Yes, absolutely. Please correct me if I'm wrong, but MRI seems to do a similar thing: https://github.com/ruby/ruby/blob/trunk/ext/date/date_parse.c#L2363

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares May 18, 2015

Member

@BanzaiMan in this case it is acceptable as date.rb has gone native somewhere along MRI 1.9.3
... simply no longer there at https://github.com/ruby/ruby/tree/ruby_1_9_3/lib (exists in ruby_1_9_2)

+ this is a follow-up on a previous date/format.rb patch: b2e1dc1?diff=unified

@azolotko test/spec will be fine under regressions ... just be aware to filter it out in 1.8 mode (still tested against under jruby-1_7)

Member

kares commented May 18, 2015

@BanzaiMan in this case it is acceptable as date.rb has gone native somewhere along MRI 1.9.3
... simply no longer there at https://github.com/ruby/ruby/tree/ruby_1_9_3/lib (exists in ruby_1_9_2)

+ this is a follow-up on a previous date/format.rb patch: b2e1dc1?diff=unified

@azolotko test/spec will be fine under regressions ... just be aware to filter it out in 1.8 mode (still tested against under jruby-1_7)

@azolotko

This comment has been minimized.

Show comment
Hide comment
@azolotko

azolotko May 21, 2015

Contributor

There seems to be some difference in meaning of :sec_fraction between _parse(str, comp) and _iso8601(str). Investigating...

Contributor

azolotko commented May 21, 2015

There seems to be some difference in meaning of :sec_fraction between _parse(str, comp) and _iso8601(str). Investigating...

@eregon

This comment has been minimized.

Show comment
Hide comment
@eregon

eregon May 21, 2015

Member

Having the spec in spec/ruby (RubySpec) would be useful as well.
If you do that, please make a separate commit for the specs.

Member

eregon commented May 21, 2015

Having the spec in spec/ruby (RubySpec) would be useful as well.
If you do that, please make a separate commit for the specs.

@azolotko azolotko closed this May 22, 2015

@azolotko azolotko reopened this May 22, 2015

@@ -937,7 +937,7 @@ def self._iso8601(str) # :nodoc:
h[:sec] = i sec if sec
end
h[:sec_fraction] = sec_fraction if sec_fraction
h[:sec_fraction] = Rational(sec_fraction.to_i, 10**sec_fraction.size) if sec_fraction

This comment has been minimized.

@eregon

eregon May 22, 2015

Member

What about Float("0#{sec_fraction}") ?

@eregon

eregon May 22, 2015

Member

What about Float("0#{sec_fraction}") ?

This comment has been minimized.

@kares

kares May 22, 2015

Member

cause of MRI compatibility:

1.9.3-p551 :002 > date = DateTime.iso8601('2014-07-08T17:51:36.013Z')
 => #<DateTime: 2014-07-08T17:51:36+00:00 ((2456847j,64296s,13000000n),+0s,2299161j)> 
1.9.3-p551 :003 > date.sec_fraction
 => (13/1000) 
1.9.3-p551 :004 > date.sec_fraction.class
 => Rational 
1.9.3-p551 :005 > date.second_fraction
 => (13/1000) 
@kares

kares May 22, 2015

Member

cause of MRI compatibility:

1.9.3-p551 :002 > date = DateTime.iso8601('2014-07-08T17:51:36.013Z')
 => #<DateTime: 2014-07-08T17:51:36+00:00 ((2456847j,64296s,13000000n),+0s,2299161j)> 
1.9.3-p551 :003 > date.sec_fraction
 => (13/1000) 
1.9.3-p551 :004 > date.sec_fraction.class
 => Rational 
1.9.3-p551 :005 > date.second_fraction
 => (13/1000) 
@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares May 22, 2015

Member

cherry-picked commit on jruby-1_7 as 4e822a1 ... great work Alex, esp. that you matched MRI ... thanks.

Member

kares commented May 22, 2015

cherry-picked commit on jruby-1_7 as 4e822a1 ... great work Alex, esp. that you matched MRI ... thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment