Skip to content
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 from
Closed

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

wants to merge 1 commit into from

Conversation

azolotko
Copy link
Contributor

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
Copy link
Member

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

@azolotko
Copy link
Contributor Author

@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
Copy link
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
Copy link
Contributor Author

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

@eregon
Copy link
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants