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

Deprecate implicit coercion of `ActiveSupport::Duration` #28204

Merged
merged 1 commit into from Mar 3, 2017

Conversation

Projects
None yet
4 participants
@pixeltrix
Member

pixeltrix commented Feb 27, 2017

Currently ActiveSupport::Duration implicitly converts to a seconds value when used in a calculation except for the explicit examples of addition and subtraction where the duration is the receiver, e.g:

>> 2 * 1.day
=> 172800

This results in lots of confusion especially when using durations with dates because adding/subtracting a value from a date treats integers as a day and not a second, e.g:

>> Date.today
=> Wed, 01 Mar 2017
>> Date.today + 2 * 1.day
=> Mon, 10 Apr 2490

To fix this we're implementing coerce so that we can provide a deprecation warning with the intent of removing the implicit coercion in Rails 5.2, e.g:

>> 2 * 1.day
DEPRECATION WARNING: Implicit coercion of ActiveSupport::Duration
to a Numeric is deprecated and will raise a TypeError in Rails 5.2.
=> 172800

In Rails 5.2 it will raise TypeError, e.g:

>> 2 * 1.day
TypeError: ActiveSupport::Duration can't be coerced into Integer

This is the same behavior as with other types in Ruby, e.g:

>> 2 * "foo"
TypeError: String can't be coerced into Integer
>> "foo" * 2
=> "foofoo"

As part of this deprecation add * and / methods to AS::Duration so that calculations that keep the duration as the receiver work
correctly whether the final receiver is a Date or Time, e.g:

>> Date.today
=> Wed, 01 Mar 2017
>> Date.today + 1.day * 2
=> Fri, 03 Mar 2017

Fixes #27457.

@@ -832,7 +832,7 @@ def test_select_date
def test_select_date_with_too_big_range_between_start_year_and_end_year
assert_raise(ArgumentError) { select_date(Time.mktime(2003, 8, 16), start_year: 2000, end_year: 20000, prefix: "date[first]", order: [:month, :day, :year]) }
assert_raise(ArgumentError) { select_date(Time.mktime(2003, 8, 16), start_year: Date.today.year - 100.years, end_year: 2000, prefix: "date[first]", order: [:month, :day, :year]) }
assert_raise(ArgumentError) { select_date(Time.mktime(2003, 8, 16), start_year: 100, end_year: 2000, prefix: "date[first]", order: [:month, :day, :year]) }

This comment has been minimized.

@pixeltrix

pixeltrix Feb 27, 2017

Member

This test exhibits the exact problem I'm trying to fix - Date.today.year - 100.years gives -3155757983.0 and not the probable intent of 1917.

This comment has been minimized.

@pixeltrix

pixeltrix Mar 15, 2017

Member

@dhh this is the problem I'm trying to solve with the deprecation - complete misunderstanding of how durations work.

@@ -1,3 +1,21 @@
* Deprecate implicit coercion of `ActiveSupport::Duration`

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 28, 2017

Member

This changelog is confusion. What are the cases where an implicit coercion is made?

This comment has been minimized.

@pixeltrix

pixeltrix Mar 1, 2017

Member

Better now?

@rafaelfranca rafaelfranca modified the milestones: 5.0.x, 5.1.0 Feb 28, 2017

@pixeltrix pixeltrix force-pushed the deprecate-implicit-coercion-of-durations branch from 6719f0b to d8430a3 Mar 1, 2017

Deprecate implicit coercion of `ActiveSupport::Duration`
Currently `ActiveSupport::Duration` implicitly converts to a seconds
value when used in a calculation except for the explicit examples of
addition and subtraction where the duration is the receiver, e.g:

    >> 2 * 1.day
    => 172800

This results in lots of confusion especially when using durations
with dates because adding/subtracting a value from a date treats
integers as a day and not a second, e.g:

    >> Date.today
    => Wed, 01 Mar 2017
    >> Date.today + 2 * 1.day
    => Mon, 10 Apr 2490

To fix this we're implementing `coerce` so that we can provide a
deprecation warning with the intent of removing the implicit coercion
in Rails 5.2, e.g:

    >> 2 * 1.day
    DEPRECATION WARNING: Implicit coercion of ActiveSupport::Duration
    to a Numeric is deprecated and will raise a TypeError in Rails 5.2.
    => 172800

In Rails 5.2 it will raise `TypeError`, e.g:

    >> 2 * 1.day
    TypeError: ActiveSupport::Duration can't be coerced into Integer

This is the same behavior as with other types in Ruby, e.g:

    >> 2 * "foo"
    TypeError: String can't be coerced into Integer
    >> "foo" * 2
    => "foofoo"

As part of this deprecation add `*` and `/` methods to `AS::Duration`
so that calculations that keep the duration as the receiver work
correctly whether the final receiver is a `Date` or `Time`, e.g:

    >> Date.today
    => Wed, 01 Mar 2017
    >> Date.today + 1.day * 2
    => Fri, 03 Mar 2017

Fixes #27457.

@pixeltrix pixeltrix force-pushed the deprecate-implicit-coercion-of-durations branch from d8430a3 to 75924c4 Mar 2, 2017

@pixeltrix pixeltrix merged commit c9bc4de into master Mar 3, 2017

2 checks passed

codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@pixeltrix pixeltrix deleted the deprecate-implicit-coercion-of-durations branch Mar 3, 2017

@@ -76,7 +76,7 @@ def test_should_enqueue_and_run_correctly_in_activejob
test "should enqueue a delivery with a delay" do
travel_to Time.new(2004, 11, 24, 01, 04, 44) do
assert_performed_with(job: ActionMailer::DeliveryJob, at: Time.current.to_f + 600.seconds, args: ["DelayedMailer", "test_message", "deliver_now", 1, 2, 3]) do
assert_performed_with(job: ActionMailer::DeliveryJob, at: Time.current.to_f + 600, args: ["DelayedMailer", "test_message", "deliver_now", 1, 2, 3]) do

This comment has been minimized.

@pixeltrix

pixeltrix Mar 15, 2017

Member

@dhh this has now been changed to Time.current + 600.seconds which it what it should've been in the first place. Also changed another test to Time.current + 1.hour instead of Time.now.to_f + 3600.

This comment has been minimized.

@dhh

dhh Mar 15, 2017

Member

Cools. I would even go with Time.current + 10.minutes. That's the wonder of these aliases. You get to pick the scale that makes the most sense.

@@ -156,7 +156,7 @@ def write_entry(key, entry, options)
expires_in = options[:expires_in].to_i
if expires_in > 0 && !options[:raw]
# Set the memcache expire a few minutes in the future to support race condition ttls on read
expires_in += 5.minutes
expires_in += 300

This comment has been minimized.

@pixeltrix

pixeltrix Mar 15, 2017

Member

This change stems from the to_i - the memcache client expects an integer for expires_in. It could be rewritten to use 5.minutes as the receiver but I thought creating an object was wasteful this deep inside the caching code.

This comment has been minimized.

@dhh

dhh Mar 15, 2017

Member

Unless we pin this as a performance bottleneck, I would always go for the clearer code. IMO, 5.minutes is much clearer than 300. This is where I really don't like to manually apply .to_i. But I could live with an alias like expires_in += 5.minutes.in_seconds.

pixeltrix added a commit that referenced this pull request Mar 15, 2017

Remove implicit coercion deprecation of durations
In #28204 we deprecated implicit conversion of durations to a
numeric which represented the number of seconds in the duration
because of unwanted side effects with calculations on durations
and dates. This unfortunately had the side effect of forcing a
explicit cast when configuring third-party libraries like
expiration in Redis, e.g:

    redis.expire("foo", 5.minutes)

To work around this we've removed the deprecation and added a
private class that wraps the numeric and can perform calculation
involving durations and ensure that they remain a duration
irrespective of the order of operations.

pixeltrix added a commit that referenced this pull request Mar 15, 2017

pixeltrix added a commit that referenced this pull request Mar 15, 2017

Remove implicit coercion deprecation of durations
In #28204 we deprecated implicit conversion of durations to a
numeric which represented the number of seconds in the duration
because of unwanted side effects with calculations on durations
and dates. This unfortunately had the side effect of forcing a
explicit cast when configuring third-party libraries like
expiration in Redis, e.g:

    redis.expire("foo", 5.minutes)

To work around this we've removed the deprecation and added a
private class that wraps the numeric and can perform calculation
involving durations and ensure that they remain a duration
irrespective of the order of operations.

pixeltrix added a commit that referenced this pull request Mar 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment