This project is archived and is in readonly mode.

ActiveSupport::Duration#* should return a Duration, not an Integer

#1916

ActiveSupport::Duration does not implement multiplication. This can lead to unexpected behaviour when the result is used later with an object that special cases Duration instances (like Date)


>> require 'activesupport'
>> d = Date.new(2000,1,1)
=> Sat, 01 Jan 2000
>> expected = d + 2.days
=> Mon, 03 Jan 2000
>> actual = d + 1.day * 2
=> Thu, 09 Feb 2473   # offset is treated as number of days, not number of seconds
>> expected == actual
=> false

as testcase:


  def test_adding_multiples_of_duration_to_date
    Date.stubs(:today).returns Date.new(2000,1,1)
    assert_equal Date.today + 2.days, Date.today + (1.day * 2)
  end

the patch by Dan Barry on #879 fixes this

http://rails.lighthouseapp.com/p...

http://rails.lighthouseapp.com/a...

Reported by Levin Alexander · February 8th, 2009 @ 08:35 PM

Activity

Levin Alexander
however, this does not make "2 * 1.day" return a duration. The only way to do that would be to overwrite Numeric#* and test for the type of the argument.

I don't think the benefits would justify the performance hit of doing that.

February 8th, 2009 @ 08:48 PM
Dan Barry
Actually, this does make 2 * 1.day return a duration. I don't know exactly why it works, but there's a test for it (test_multiplication_associativity).

February 9th, 2009 @ 01:04 AM
Levin Alexander
no, it does not. This test fails:
```
  def test_multiplication_associativity
    assert_equal "2.months", (1.month * 2).inspect
    assert_equal "2.months", (2 * 1.month).inspect
  end
```
it just looks like it works, because durations are converted to seconds, before they are compared.
February 9th, 2009 @ 01:32 AM
Levin Alexander
there is a small error in the test. It has to say "2 months" (without dot) instead of "2.months"

February 9th, 2009 @ 01:40 AM
Dan Barry
Ah, good catch. I'll have to update the tests and figure that out.

February 13th, 2009 @ 08:18 PM
Levin Alexander
Attached is just the Duration#* part of your (Dan's) patch, because that part seems uncontroversial.

This patch does not change behavior of Numeric*Duration and it does not implement Duration#/

It also does not check if the multiplier is a Fixnum because it is not clear what the fallback should be if it isn't (could call "to_i", "round", raise an argument error)
- duration_multiply.diff 1.71 KB
February 14th, 2009 @ 07:29 PM
Pratik
- Assigned user set to Geoff Buesing
- Tag changed from active_support, duration to active_support, duration, patch
March 8th, 2009 @ 02:57 PM
Geoff Buesing
- State changed from new to wontfix
Unsure about this one. Reasons:
1. With this change, 1.day * 2 returns a Duration, but 2 * 1.day returns a Fixnum. Seems odd and unexpected.
2. We disallow creating a Duration via Float#months and #years (e.g., you can no longer do "5.3.years"), but this patch would allow you to mutliply by a Float (e.g. "1.year * 5.3")
January 28th, 2010 @ 03:15 AM