This project is archived and is in readonly mode.

#1916 ✓wontfix
Levin Alexander

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

Reported by Levin Alexander | February 8th, 2009 @ 08:35 PM | in 2.x

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

Comments and changes to this ticket

  • Levin Alexander

    Levin Alexander February 8th, 2009 @ 08:48 PM

    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.

  • Dan Barry

    Dan Barry February 9th, 2009 @ 01:04 AM

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

  • Levin Alexander

    Levin Alexander February 9th, 2009 @ 01:32 AM

    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.

  • Levin Alexander

    Levin Alexander February 9th, 2009 @ 01:40 AM

    there is a small error in the test. It has to say "2 months" (without dot) instead of "2.months"

  • Dan Barry

    Dan Barry February 13th, 2009 @ 08:18 PM

    Ah, good catch. I'll have to update the tests and figure that out.

  • Levin Alexander

    Levin Alexander February 14th, 2009 @ 07:29 PM

    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)

  • Pratik

    Pratik March 8th, 2009 @ 02:57 PM

    • Assigned user set to “Geoff Buesing”
    • Tag changed from “active_support, duration” to “active_support, duration, patch”
  • Geoff Buesing

    Geoff Buesing January 28th, 2010 @ 03:15 AM

    • 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")

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

People watching this ticket

Attachments

Tags

Referenced by

Pages