diff options
| -rw-r--r-- | activesupport/CHANGELOG.md | 22 | ||||
| -rw-r--r-- | activesupport/lib/active_support/duration.rb | 48 | ||||
| -rw-r--r-- | activesupport/test/core_ext/duration_test.rb | 75 |
3 files changed, 131 insertions, 14 deletions
diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md index 42d3467e7d..938d4f9d31 100644 --- a/activesupport/CHANGELOG.md +++ b/activesupport/CHANGELOG.md @@ -1,3 +1,25 @@ +* Fix modulo operations involving durations + + Rails 5.1 introduce an `ActiveSupport::Duration::Scalar` class as a wrapper + around a numeric value as a way of ensuring a duration was the outcome of + an expression. However the implementation was missing support for modulo + operations. This support has now been added and should result in a duration + being returned from expressions involving modulo operations. + + Prior to Rails 5.1: + + 5.minutes % 2.minutes + => 60 + + Now: + + 5.minutes % 2.minutes + => 1 minute + + Fixes #29603 and #29743. + + *Sayan Chakraborty*, *Andrew White* + * Fix division where a duration is the denominator PR #29163 introduced a change in behavior when a duration was the denominator diff --git a/activesupport/lib/active_support/duration.rb b/activesupport/lib/active_support/duration.rb index 0d45566d43..f411bb81df 100644 --- a/activesupport/lib/active_support/duration.rb +++ b/activesupport/lib/active_support/duration.rb @@ -82,6 +82,14 @@ module ActiveSupport end end + def %(other) + if Duration === other + Duration.build(value % other.value) + else + calculate(:%, other) + end + end + private def calculate(op, other) if Scalar === other @@ -115,6 +123,8 @@ module ActiveSupport years: SECONDS_PER_YEAR }.freeze + PARTS = [:years, :months, :weeks, :days, :hours, :minutes, :seconds].freeze + attr_accessor :value, :parts autoload :ISO8601Parser, "active_support/duration/iso8601_parser" @@ -165,6 +175,30 @@ module ActiveSupport new(value * SECONDS_PER_YEAR, [[:years, value]]) end + # Creates a new Duration from a seconds value that is converted + # to the individual parts: + # + # ActiveSupport::Duration.build(31556952).parts # => {:years=>1} + # ActiveSupport::Duration.build(2716146).parts # => {:months=>1, :days=>1} + # + def build(value) + parts = {} + remainder = value.to_f + + PARTS.each do |part| + unless part == :seconds + part_in_seconds = PARTS_IN_SECONDS[part] + parts[part] = remainder.div(part_in_seconds) + remainder = (remainder % part_in_seconds).round(9) + end + end + + parts[:seconds] = remainder + parts.reject! { |k, v| v.zero? } + + new(value, parts) + end + private def calculate_total_seconds(parts) @@ -242,6 +276,18 @@ module ActiveSupport end end + # Returns the modulo of this Duration by another Duration or Numeric. + # Numeric values are treated as seconds. + def %(other) + if Duration === other || Scalar === other + Duration.build(value % other.value) + elsif Numeric === other + Duration.build(value % other) + else + raise_type_error(other) + end + end + def -@ #:nodoc: Duration.new(-value, parts.map { |type, number| [type, -number] }) end @@ -326,7 +372,7 @@ module ActiveSupport def inspect #:nodoc: parts. reduce(::Hash.new(0)) { |h, (l, r)| h[l] += r; h }. - sort_by { |unit, _ | [:years, :months, :weeks, :days, :hours, :minutes, :seconds].index(unit) }. + sort_by { |unit, _ | PARTS.index(unit) }. map { |unit, val| "#{val} #{val == 1 ? unit.to_s.chop : unit.to_s}" }. to_sentence(locale: ::I18n.default_locale) end diff --git a/activesupport/test/core_ext/duration_test.rb b/activesupport/test/core_ext/duration_test.rb index 1d607a20a6..b3e0cd8bd0 100644 --- a/activesupport/test/core_ext/duration_test.rb +++ b/activesupport/test/core_ext/duration_test.rb @@ -111,7 +111,44 @@ class DurationTest < ActiveSupport::TestCase def test_divide assert_equal 1.day, 7.days / 7 assert_instance_of ActiveSupport::Duration, 7.days / 7 + + assert_equal 1.hour, 1.day / 24 + assert_instance_of ActiveSupport::Duration, 1.day / 24 + + assert_equal 24, 86400 / 1.hour + assert_kind_of Integer, 86400 / 1.hour + + assert_equal 24, 1.day / 1.hour + assert_kind_of Integer, 1.day / 1.hour + assert_equal 1, 1.day / 1.day + assert_kind_of Integer, 1.day / 1.hour + end + + def test_modulo + assert_equal 1.minute, 5.minutes % 120 + assert_instance_of ActiveSupport::Duration, 5.minutes % 120 + + assert_equal 1.minute, 5.minutes % 2.minutes + assert_instance_of ActiveSupport::Duration, 5.minutes % 2.minutes + + assert_equal 1.minute, 5.minutes % 120.seconds + assert_instance_of ActiveSupport::Duration, 5.minutes % 120.seconds + + assert_equal 5.minutes, 5.minutes % 1.hour + assert_instance_of ActiveSupport::Duration, 5.minutes % 1.hour + + assert_equal 1.day, 36.days % 604800 + assert_instance_of ActiveSupport::Duration, 36.days % 604800 + + assert_equal 1.day, 36.days % 7.days + assert_instance_of ActiveSupport::Duration, 36.days % 7.days + + assert_equal 800.seconds, 8000 % 1.hour + assert_instance_of ActiveSupport::Duration, 8000 % 1.hour + + assert_equal 1.month, 13.months % 1.year + assert_instance_of ActiveSupport::Duration, 13.months % 1.year end def test_date_added_with_multiplied_duration @@ -411,8 +448,6 @@ class DurationTest < ActiveSupport::TestCase assert_instance_of ActiveSupport::Duration::Scalar, scalar / 2 assert_equal 10, 100.seconds / scalar assert_instance_of ActiveSupport::Duration, 100.seconds / scalar - assert_equal 20, 2.seconds * scalar - assert_instance_of ActiveSupport::Duration, 2.seconds * scalar assert_equal 5, scalar / 2.seconds assert_kind_of Integer, scalar / 2.seconds @@ -423,6 +458,31 @@ class DurationTest < ActiveSupport::TestCase assert_equal "no implicit conversion of String into ActiveSupport::Duration::Scalar", exception.message end + def test_scalar_modulo + scalar = ActiveSupport::Duration::Scalar.new(10) + + assert_equal 1, 31 % scalar + assert_instance_of ActiveSupport::Duration::Scalar, 31 % scalar + assert_equal 1, scalar % 3 + assert_instance_of ActiveSupport::Duration::Scalar, scalar % 3 + assert_equal 1, 31.seconds % scalar + assert_instance_of ActiveSupport::Duration, 31.seconds % scalar + assert_equal 1, scalar % 3.seconds + assert_instance_of ActiveSupport::Duration, scalar % 3.seconds + + exception = assert_raises(TypeError) do + scalar % "foo" + end + + assert_equal "no implicit conversion of String into ActiveSupport::Duration::Scalar", exception.message + end + + def test_scalar_modulo_parts + scalar = ActiveSupport::Duration::Scalar.new(82800) + assert_equal({ hours: 1 }, (scalar % 2.hours).parts) + assert_equal(3600, (scalar % 2.hours).value) + end + def test_twelve_months_equals_one_year assert_equal 12.months, 1.year end @@ -431,17 +491,6 @@ class DurationTest < ActiveSupport::TestCase assert_not_equal 30.days, 1.month end - def test_division - assert_equal 1.hour, 1.day / 24 - assert_instance_of ActiveSupport::Duration, 1.day / 24 - - assert_equal 24, 86400 / 1.hour - assert_kind_of Integer, 86400 / 1.hour - - assert_equal 24, 1.day / 1.hour - assert_kind_of Integer, 1.day / 1.hour - end - def test_adding_one_month_maintains_day_of_month (1..11).each do |month| [1, 14, 28].each do |day| |