Fix cannot load complex into simple error when loading marshal dump (Fixes #20)

[jeremyevans]

No description provided.

[hsbt]

Thanks, I will release 3.0.1 after merging this.

[nobu]

By adding a Rational, returning an invalid Date is broken by design.
I think this patch would direct wrong direction.

[jeremyevans]

Well, I agree that a Date with a fractional day makes very little sense. However, those have always been allowed, so we would have to drop backwards compatibility if we want to prevent Dates with fractional days. That's a much more involved change than this patch.

[nobu]

In a fraction case, it should be a DateTime instead of a Date, I think.
Or simply truncate the fractional part.

[nobu]

I think this is an analogue to the relationship between Integer and Rational/Float.
Adding a Rational to an Integer doesn't make an Integer with a fractional part, but a new Rational.

[jeremyevans]

So one of?:

Date.jd 1/2r
# => #<DateTime: -4712-01-01T12:00:00+00:00 ((0j,43200s,0n),+0s,2299161j)>
# => #<Date: -4712-01-01 ((0j,0s,0n),+0s,2299161j)>

[nobu]

Yes, ...but it sounds also curious that a class method to create a Date instance returns a DateTime...

[jeremyevans]

Well if we can agree on behavior, I can work on implementing them in preparation for date 4.0.0. However, in terms of fixing date 3.0.0, I think the pull request should be merged.

[nobu]

In the long term, I'd love to drop date.
We haven't do it yet just as time.rb depends on Date._parse kitchen sink.

[jeremyevans]

DateTime doesn't add that much value anymore, since Ruby 2.6 at least, though some people may prefer it's date-based API as opposed to Time's second-based API. However, Date itself fills a very common need and I can't imagine dropping it from stdlib.

[nobu]

In short term, we should remove SimpleDateData.
https://github.com/nobu/date/tree/remove-SimpleDate

[jeremyevans]

I believe the reason for SimpleDateData and ComplexDateData (the "switch hitter" implementation) is for performance. Just removing SimpleDateData could result in lower performance. Are there significant performance differences in your remove-SimpleDate tree?

[nobu]

Just for Date#+

$ ruby -rdate -rbenchmark -e 'd = Date.today; Benchmark.bm {|x|3.times {x.report{1_000_000.times {d+1}}}}'
       user     system      total        real
   0.813685   0.002375   0.816060 (  0.817448)
   0.787374   0.001083   0.788457 (  0.788966)
   0.772593   0.000657   0.773250 (  0.773630)

$ ruby -I./lib -rdate -rbenchmark -e 'd = Date.today; Benchmark.bm {|x|3.times {x.report{1_000_000.times {d+1}}}}'
       user     system      total        real
   0.793767   0.002009   0.795776 (  0.796659)
   0.791993   0.000998   0.792991 (  0.793701)
   0.805675   0.000646   0.806321 (  0.806839)

Seems not significant.

[nobu]

Regarding the original issue, I think “a fractional date” should be warned at least.

[jeremyevans]

Date.new(year, month, day) is the more important benchmark, as the vast majority of Date objects are created but have no calculations performed on them.

I'm fine warning for fractional date, especially if we plan to remove support for it in the next major version.

[nobu]

Similar results.

$ ruby -rdate -rbenchmark -e 'Benchmark.bm {|x|3.times {x.report{1_000_000.times {Date.new(2020,6,12)}}}}'
       user     system      total        real
   1.541138   0.003184   1.544322 (  1.546589)
   1.532344   0.002877   1.535221 (  1.537473)
   1.479800   0.002416   1.482216 (  1.484957)

$ ruby -I./lib -rdate -rbenchmark -e 'Benchmark.bm {|x|3.times {x.report{1_000_000.times {Date.new(2020,6,12)}}}}'
       user     system      total        real
   1.466163   0.003181   1.469344 (  1.471213)
   1.531565   0.002866   1.534431 (  1.536860)
   1.539822   0.002146   1.541968 (  1.544386)

[zdavatz]

@nobu if you remove Date you have to make sure that you do not break Userspace and that backwards compatibility is guaranteed. I remember at least one Rubykaigi discussion with Matz where you (Ruby developers) broke the Regular Expression (Oniguruma Patch that was never applied) switching from Ruby 1.8.6 to Ruby 1.9.3. - that was like a major user space breakage. After that huge inconsistency in Ruby I really tried to convince Matz to adopt the philosophy of Linus and Linux to never break the Userspace. So please keep this in mind before your remove important features of Ruby. Consistency is key!

[zdavatz]

@jeremyevans how does boost deal with dates?

[nobu]

https://github.com/nobu/date/tree/promote-to-complex

[ioquatix]

Why not just make Date handle all functionality and make DateTime = Date.

[jeremyevans]

@ioquatix Dates are fundamentally different than DateTimes.

@zdavatz No idea how Boost deals with dates. It's open source, I'm sure you can look at it if it interests you.

[jeremyevans]

@nobu I reviewed your promote-to-complex tree and am fine using that approach instead of the one in this pull request. The only suggestion I would make is dropping the warning. If we want to warn for fractional dates, we should do that in a separate commit that handles all cases, not just when marshalling.

[nobu]

I agree that the warning is not useful.

BTW, is it intentional that the added test can pass even if the unmarshaled object is not an instance of Date?

 	RTYPEDDATA(self)->data = dat;
+	RB_OBJ_WRITE(self, &RBASIC(self)->klass, cDateTime);
 	goto complex_data;

[jeremyevans]

We could add assert_instance_of(Date, d2) to test that. I think it would be a good idea to add a check similar to that in all places in the test method where we currently only use assert_instance_of(String, d2.to_s)

[nobu]

Why not just make Date handle all functionality and make DateTime = Date.

It didn't work at all as DateTime has many overriding methods, rake failed with 14F12E. 😞

[nobu]

And I tried auto conversion, making Date methods to return a DateTime instead of a fractional Date, but it resulted in 2 failures in test_date_conv.rb.

[zdavatz]

@nobu how is it going? Any chances of getting date-3.0.1.gem any time soon?