Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Nov 27, 2013

Conversation

@chancancode
Copy link
Member

@chancancode chancancode commented Sep 27, 2013

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.

/cc @jeremy @sikachu

@jeremy
jeremy reviewed Sep 27, 2013
View changes
activesupport/CHANGELOG.md Outdated
@@ -1,3 +1,7 @@
* Deprecated Numeric#{ago,until,since,from_now}, use 5.seconds.ago instead.

This comment has been minimized.

@jeremy

jeremy Sep 27, 2013
Member

The pull request description is so clear and understandable, but this just describes the diff. Think of the Rails dev reviewing the CHANGELOG to see what's changed and why.

This comment has been minimized.

@chancancode

chancancode Sep 27, 2013
Author Member

Fixed 😁

@jeremy
jeremy reviewed Sep 27, 2013
View changes
activesupport/test/core_ext/numeric_ext_test.rb Outdated
assert seconds.from_now >= now + seconds
assert_deprecated do
now = Time.now
assert seconds.ago >= now - seconds

This comment has been minimized.

@jeremy

jeremy Sep 27, 2013
Member

Ruby already has Time#- whose argument is numeric seconds, so we should not deprecate it.

This comment has been minimized.

@chancancode

chancancode Sep 27, 2013
Author Member

The deprecation here is from LHS (seconds.ago) because seconds is actually an integer here, I'll change the name of the block arg here to be explicit.

@jeremy
jeremy reviewed Sep 27, 2013
View changes
activesupport/test/core_ext/numeric_ext_test.rb Outdated
assert_equal 365.25.days.to_f.since(@dtnow), 1.year.to_f.since(@dtnow)
def test_duration_loses_accuracy_after_conversion
assert_equal 30.days.to_i, 1.month.to_i
assert_equal 365.25.days.to_f, 1.year.to_f
end

This comment has been minimized.

This comment has been minimized.

@chancancode

chancancode Sep 27, 2013
Author Member

I was going to wrap these with assert_deprecated (because .to_i.since will trigger a deprecation warning), but if I do that, the test will likely be removed when someone eventually remove Numeric.since in the future. So I tried to figure out the intent of these tests and rewrote them.

I changed them to be 30.days.to_i.seconds.since in the latest rev, is that better?

@sikachu
sikachu reviewed Sep 27, 2013
View changes
activesupport/test/core_ext/numeric_ext_test.rb Outdated
@@ -94,11 +97,11 @@ def test_since_and_ago_anchored_to_time_now_when_time_zone_is_not_set
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_equal false, 5.seconds.since.is_a?(ActiveSupport::TimeWithZone)

This comment has been minimized.

@sikachu

sikachu Sep 27, 2013
Member

Would you mind refactor all of these is_a? to use assert_instance_of?

This comment has been minimized.

@chancancode

chancancode Sep 27, 2013
Author Member

@sikachu turns out that after my change, these test are character-by-character identical to the test in durations_test, so I fixed it there and removed these. The alternative is that I can keep these old tests, and wrap them in assert_deprecated, then remove later (but they will both be testing the same thing). WDYT?

@jeremy
jeremy reviewed Sep 27, 2013
View changes
activesupport/lib/active_support/core_ext/numeric/time.rb Outdated
@@ -63,7 +63,8 @@ def fortnights

# Reads best without arguments: 10.minutes.ago
def ago(time = ::Time.current)
time - self
ActiveSupport::Deprecation.warn "Numeric#ago (#{self.to_s}.ago) is deprecated and will be removed in the future, use #{self.to_s}.seconds.ago instead"

This comment has been minimized.

@jeremy

jeremy Sep 27, 2013
Member

This is going to say Numeric#ago (2013-09-27 14:36:52 -0700.ago) is deprecated ... which doesn't make sense. Consider something like: "Calling the #ago method on a number, like 5.ago, is deprecated in favor of calling it on a duration, like 5.seconds.ago."

def test_intervals
@seconds.values.each do |seconds|
assert_equal seconds.since(@now), @now + seconds
assert_equal seconds.until(@now), @now - seconds

This comment has been minimized.

@jeremy

jeremy Sep 27, 2013
Member

Time#+ and Time#- should not be deprecated, since they're plain Ruby. This tests that + and - behave correctly; it's not testing since/ago. Can fix by doing: assert_equal seconds.seconds.since(@now), @now + seconds

This comment has been minimized.

@chancancode

chancancode Nov 6, 2013
Author Member

As discussed on campfire, the original test was wrong (arguments were flipped)

@jeremy
jeremy reviewed Sep 27, 2013
View changes
activesupport/test/core_ext/numeric_ext_test.rb Outdated
now = Time.now
assert seconds.from_now >= now + seconds
from_now = assert_deprecated { seconds_as_integer.from_now }
assert from_now >= now + seconds_as_integer

This comment has been minimized.

@jeremy

jeremy Sep 27, 2013
Member

Same. These should not be deprecated.

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)

This comment has been minimized.

@jeremy
jeremy reviewed Sep 27, 2013
View changes
activesupport/test/core_ext/numeric_ext_test.rb Outdated
ensure
Time.zone = nil
end

This comment has been minimized.

@jeremy

jeremy Sep 27, 2013
Member

Why are these deleted?

This comment has been minimized.

@chancancode

chancancode Sep 27, 2013
Author Member

They were identical to durations_test, but I'm reverting this

@chancancode
Copy link
Member Author

@chancancode chancancode commented Sep 27, 2013

@jeremy I trimmed this down to an MVP / Minimal Viable Pull-request. Doubts-- ? 😁

@jeremy
jeremy reviewed Sep 27, 2013
View changes
activesupport/lib/active_support/core_ext/numeric/time.rb Outdated
@@ -63,6 +63,7 @@ def fortnights

# Reads best without arguments: 10.minutes.ago
def ago(time = ::Time.current)
ActiveSupport::Deprecation.warn "Calling #ago or #until on a nunmber (e.g. 5.ago) is deprecated and will be removed in the future, use 5.seconds.ago instead"

This comment has been minimized.

@jeremy

jeremy Sep 27, 2013
Member

nunmber -> number

@jeremy
jeremy reviewed Sep 27, 2013
View changes
activesupport/lib/active_support/core_ext/numeric/time.rb Outdated
@@ -71,6 +72,7 @@ def ago(time = ::Time.current)

# Reads best with argument: 10.minutes.since(time)
def since(time = ::Time.current)
ActiveSupport::Deprecation.warn "Calling #since or #from_now on a nunmber (e.g. 5.since) is deprecated and will be removed in the future, use 5.seconds.since instead"

This comment has been minimized.

@jeremy

jeremy Sep 27, 2013
Member

nunmber -> number

@jeremy
jeremy reviewed Sep 27, 2013
View changes
activesupport/test/core_ext/numeric_ext_test.rb Outdated

# Numeric#{since,until} currently goes through a different code path than AS::Duration#{since,until}
# so the two tests above are duplicated below

This comment has been minimized.

@jeremy

jeremy Sep 27, 2013
Member

✂️ whitespace

@jeremy
jeremy reviewed Sep 27, 2013
View changes
activesupport/test/core_ext/numeric_ext_test.rb Outdated
assert_equal assert_deprecated { seconds.since(@now) }, @now + seconds
assert_equal assert_deprecated { seconds.until(@now) }, @now - seconds
end
end

This comment has been minimized.

@jeremy

jeremy Sep 27, 2013
Member

Are since and until covered by other tests? Does this need to be duplicated?

This comment has been minimized.

@chancancode

chancancode Sep 27, 2013
Author Member

This is testing whatever it was testing – in the original test, it was testing that seconds.since(@now) should equal @now + seconds. Now that I changed the original test to seconds.seconds.since(@now), it goes through a different code path (it's now testing AS::Duration#since instead), and the original code path (Numeric#since) is untested. Hence the duplication (and also why I changed Numeric#since to use AS::Duration in the first pass)

This comment has been minimized.

@chancancode

chancancode Sep 27, 2013
Author Member

Re: are they covered in other tests – all the tests that actually tests Numeric#since / #until must be wrapped in assert_deprecated so as far as I can tell, this and the time with zone stuff is the only places that actually testes Numeric#since / #until. They might be indirectly tested via AS::Duration, but the implementation was slightly different so I wasn't too comfortable about removing coverage.

@jeremy
jeremy reviewed Sep 27, 2013
View changes
activesupport/test/core_ext/numeric_ext_test.rb Outdated
now = Time.now
assert seconds.from_now >= now + seconds
from_now = assert_deprecated { seconds.from_now }
assert from_now >= now + seconds
end
end

This comment has been minimized.

@jeremy

jeremy Sep 27, 2013
Member

Same. What is this testing?

This comment has been minimized.

@chancancode

chancancode Sep 27, 2013
Author Member

ditto (same for the time_with_zone tests below, there are almost-identical tests in duration_test.rb, but I am keeping this to test Numeric#since)

now = Time.now
assert assert_deprecated { 1.since } >= now + 1
now = Time.now
assert assert_deprecated { 1.ago } >= now - 1
end

This comment has been minimized.

@chancancode

chancancode Nov 6, 2013
Author Member

ditto

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.
@chancancode
Copy link
Member Author

@chancancode chancancode commented Nov 27, 2013

Rebased to keep it fresh :) Probably would have to do it again when someone is ready to merge this, stupid CHANGELOGs

@jeremy
Copy link
Member

@jeremy jeremy commented Nov 27, 2013

Merged d4016f2

@jeremy jeremy closed this Nov 27, 2013
@jeremy jeremy merged commit 1f16136 into rails:master Nov 27, 2013
1 check passed
1 check passed
default The Travis CI build passed
Details
chancancode added a commit that referenced this pull request May 30, 2014
Replacements:

   5.ago   => 5.seconds.ago
   5.until => 5.seconds.until
   5.since => 5.seconds.since
   5.from_now => 5.seconds.from_now

The removed tests does not affect coverage – we have equivalent test cases in
the tests for `AS::Duration`.

See #12389 for the history and rationale behind this.
jrafanie added a commit to tenderlove/manageiq that referenced this pull request Jan 8, 2015
Fixes: undefined method `from_now' for 600:Fixnum

See also: rails/rails#12389
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.