Deprecated Numeric#{ago,until,since,from_now}

The user is expected to explicitly convert the value into an
AS::Duration, i.e. `5.ago` => `5.seconds.ago`

This will help to catch subtle bugs like:

  def recent?(days = 3)
    self.created_at >= days.ago
  end

The above code would check if the model is created within the last 3
**seconds**.

In the future, `Numeric#{ago,until,since,from_now}` should be removed
completely, or throw some sort of errors to indicate there are no
implicit conversion from `Numeric` to `AS::Duration`.

Also fixed & refactor the test cases for Numeric#{ago,since} and
AS::Duration#{ago,since}. The original test case had the assertion
flipped and the purpose of the test wasn't very clear.

4 files changed, 58 insertions, 31 deletions

diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md
index 104b5eafa1..d03b1674d4 100644
--- a/activesupport/CHANGELOG.md
+++ b/activesupport/CHANGELOG.md
@@ -1,3 +1,20 @@
+*   Deprecated Numeric#{ago,until,since,from_now}, the user is expected to explicitly
+    convert the value into an AS::Duration, i.e. 5.ago => 5.seconds.ago
+
+    This will help to catch subtle bugs like:
+
+      def recent?(days = 3)
+        self.created_at >= days.ago
+      end
+
+    The above code would check if the model is created within the last 3 **seconds**.
+
+    In the future, Numeric#{ago,until,since,from_now} should be removed completely,
+    or throw some sort of errors to indicate there are no implicit conversion from
+    Numeric to AS::Duration.
+
+    *Godfrey Chan*
+
 *   Add `ActiveSupport::Testing::TimeHelpers#travel` and `#travel_to`. These methods change current
     time to the given time or time difference by stubbing `Time.now` and `Date.today` to return the
     time or date after the difference calculation, or the time or date that got passed into the
diff --git a/activesupport/lib/active_support/core_ext/numeric/time.rb b/activesupport/lib/active_support/core_ext/numeric/time.rb
index d97ce938c1..704c4248d9 100644
--- a/activesupport/lib/active_support/core_ext/numeric/time.rb
+++ b/activesupport/lib/active_support/core_ext/numeric/time.rb
@@ -63,6 +63,7 @@ class Numeric
 
   # Reads best without arguments:  10.minutes.ago
   def ago(time = ::Time.current)
+    ActiveSupport::Deprecation.warn "Calling #ago or #until on a number (e.g. 5.ago) is deprecated and will be removed in the future, use 5.seconds.ago instead"
     time - self
   end
 
@@ -71,6 +72,7 @@ class Numeric
 
   # Reads best with argument:  10.minutes.since(time)
   def since(time = ::Time.current)
+    ActiveSupport::Deprecation.warn "Calling #since or #from_now on a number (e.g. 5.since) is deprecated and will be removed in the future, use 5.seconds.since instead"
     time + self
   end
 
diff --git a/activesupport/test/core_ext/duration_test.rb b/activesupport/test/core_ext/duration_test.rb
index ed267cf4b9..47257debe4 100644
--- a/activesupport/test/core_ext/duration_test.rb
+++ b/activesupport/test/core_ext/duration_test.rb
@@ -71,6 +71,19 @@ class DurationTest < ActiveSupport::TestCase
     assert_equal 86400 * 1.7, 1.7.days
   end
 
+  def test_since_and_ago
+    t = Time.local(2000)
+    assert t + 1, 1.second.since(t)
+    assert t - 1, 1.second.ago(t)
+  end
+
+  def test_since_and_ago_without_argument
+    now = Time.now
+    assert 1.second.since >= now + 1
+    now = Time.now
+    assert 1.second.ago >= now - 1
+  end
+
   def test_since_and_ago_with_fractional_days
     t = Time.local(2000)
     # since
@@ -96,10 +109,10 @@ class DurationTest < ActiveSupport::TestCase
     with_env_tz 'US/Eastern' do
       Time.stubs(:now).returns Time.local(2000)
       # since
-      assert_equal false, 5.seconds.since.is_a?(ActiveSupport::TimeWithZone)
+      assert_not_instance_of ActiveSupport::TimeWithZone, 5.seconds.since
       assert_equal Time.local(2000,1,1,0,0,5), 5.seconds.since
       # ago
-      assert_equal false, 5.seconds.ago.is_a?(ActiveSupport::TimeWithZone)
+      assert_not_instance_of ActiveSupport::TimeWithZone, 5.seconds.ago
       assert_equal Time.local(1999,12,31,23,59,55), 5.seconds.ago
     end
   end
@@ -109,11 +122,11 @@ class DurationTest < ActiveSupport::TestCase
     with_env_tz 'US/Eastern' do
       Time.stubs(:now).returns Time.local(2000)
       # since
-      assert_equal true, 5.seconds.since.is_a?(ActiveSupport::TimeWithZone)
+      assert_instance_of ActiveSupport::TimeWithZone, 5.seconds.since
       assert_equal Time.utc(2000,1,1,0,0,5), 5.seconds.since.time
       assert_equal 'Eastern Time (US & Canada)', 5.seconds.since.time_zone.name
       # ago
-      assert_equal true, 5.seconds.ago.is_a?(ActiveSupport::TimeWithZone)
+      assert_instance_of ActiveSupport::TimeWithZone, 5.seconds.ago
       assert_equal Time.utc(1999,12,31,23,59,55), 5.seconds.ago.time
       assert_equal 'Eastern Time (US & Canada)', 5.seconds.ago.time_zone.name
     end
diff --git a/activesupport/test/core_ext/numeric_ext_test.rb b/activesupport/test/core_ext/numeric_ext_test.rb
index 23bbb8d7d2..3b1dabea8d 100644
--- a/activesupport/test/core_ext/numeric_ext_test.rb
+++ b/activesupport/test/core_ext/numeric_ext_test.rb
@@ -22,21 +22,16 @@ class NumericExtTimeAndDateTimeTest < ActiveSupport::TestCase
     end
   end
 
-  def test_intervals
-    @seconds.values.each do |seconds|
-      assert_equal seconds.since(@now), @now + seconds
-      assert_equal seconds.until(@now), @now - seconds
-    end
+  def test_deprecated_since_and_ago
+    assert_equal @now + 1, assert_deprecated { 1.since(@now) }
+    assert_equal @now - 1, assert_deprecated { 1.ago(@now) }
   end
 
-  # Test intervals based from Time.now
-  def test_now
-    @seconds.values.each do |seconds|
-      now = Time.now
-      assert seconds.ago >= now - seconds
-      now = Time.now
-      assert seconds.from_now >= now + seconds
-    end
+  def test_deprecated_since_and_ago_without_argument
+    now = Time.now
+    assert assert_deprecated { 1.since } >= now + 1
+    now = Time.now
+    assert assert_deprecated { 1.ago } >= now - 1
   end
 
   def test_irregular_durations
@@ -78,10 +73,10 @@ class NumericExtTimeAndDateTimeTest < ActiveSupport::TestCase
   end
 
   def test_duration_after_conversion_is_no_longer_accurate
-    assert_equal 30.days.to_i.since(@now), 1.month.to_i.since(@now)
-    assert_equal 365.25.days.to_f.since(@now), 1.year.to_f.since(@now)
-    assert_equal 30.days.to_i.since(@dtnow), 1.month.to_i.since(@dtnow)
-    assert_equal 365.25.days.to_f.since(@dtnow), 1.year.to_f.since(@dtnow)
+    assert_equal 30.days.to_i.seconds.since(@now), 1.month.to_i.seconds.since(@now)
+    assert_equal 365.25.days.to_f.seconds.since(@now), 1.year.to_f.seconds.since(@now)
+    assert_equal 30.days.to_i.seconds.since(@dtnow), 1.month.to_i.seconds.since(@dtnow)
+    assert_equal 365.25.days.to_f.seconds.since(@dtnow), 1.year.to_f.seconds.since(@dtnow)
   end
 
   def test_add_one_year_to_leap_day
@@ -94,11 +89,11 @@ class NumericExtTimeAndDateTimeTest < ActiveSupport::TestCase
     with_env_tz 'US/Eastern' do
       Time.stubs(:now).returns Time.local(2000)
       # since
-      assert_equal false, 5.since.is_a?(ActiveSupport::TimeWithZone)
-      assert_equal Time.local(2000,1,1,0,0,5), 5.since
+      assert_not_instance_of ActiveSupport::TimeWithZone, assert_deprecated { 5.since }
+      assert_equal Time.local(2000,1,1,0,0,5), assert_deprecated { 5.since }
       # ago
-      assert_equal false, 5.ago.is_a?(ActiveSupport::TimeWithZone)
-      assert_equal Time.local(1999,12,31,23,59,55), 5.ago
+      assert_not_instance_of ActiveSupport::TimeWithZone, assert_deprecated { 5.ago }
+      assert_equal Time.local(1999,12,31,23,59,55), assert_deprecated { 5.ago }
     end
   end
 
@@ -107,13 +102,13 @@ class NumericExtTimeAndDateTimeTest < ActiveSupport::TestCase
     with_env_tz 'US/Eastern' do
       Time.stubs(:now).returns Time.local(2000)
       # since
-      assert_equal true, 5.since.is_a?(ActiveSupport::TimeWithZone)
-      assert_equal Time.utc(2000,1,1,0,0,5), 5.since.time
-      assert_equal 'Eastern Time (US & Canada)', 5.since.time_zone.name
+      assert_instance_of ActiveSupport::TimeWithZone, assert_deprecated { 5.since }
+      assert_equal Time.utc(2000,1,1,0,0,5), assert_deprecated { 5.since.time }
+      assert_equal 'Eastern Time (US & Canada)',  assert_deprecated { 5.since.time_zone.name }
       # ago
-      assert_equal true, 5.ago.is_a?(ActiveSupport::TimeWithZone)
-      assert_equal Time.utc(1999,12,31,23,59,55), 5.ago.time
-      assert_equal 'Eastern Time (US & Canada)', 5.ago.time_zone.name
+      assert_instance_of ActiveSupport::TimeWithZone, assert_deprecated { 5.ago }
+      assert_equal Time.utc(1999,12,31,23,59,55), assert_deprecated { 5.ago.time }
+      assert_equal 'Eastern Time (US & Canada)', assert_deprecated { 5.ago.time_zone.name }
     end
   ensure
     Time.zone = nil