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

Replace AS::TimeWithZone#since with alias to + #18063

Merged
merged 1 commit into from Dec 17, 2014

Conversation

claudiob
Copy link
Member

Stems from Google group discussion.

Currently AS::TimeWithZone has two methods to add an interval to a time: +(other) and since(other) (docs).

The two methods are "pretty much" equivalent in every case:

  1. When adding any interval to an AS::TimeWithZone representing a Time:

    t = Time.now.in_time_zone       #=> Thu, 04 Dec 2014 18:56:28 EST -05:00
    t + 1 == t.since(1)             #=> true
    t + 1.day == t.since(1.day)     #=> true
    t + 1.month == t.since(1.month) #=> true
  2. When adding any interval to an AS::TimeWithZone representing a Date:

    d = Date.today.in_time_zone     #=> Thu, 04 Dec 2014 00:00:00 EST -05:00
    d + 1 == d.since(1)             #=> true
    d + 1.day == d.since(1.day)     #=> true
    d + 1.month == d.since(1.month) #=> true
  3. When adding any interval to an AS::TimeWithZone representing a DateTime:

    dt = DateTime.now.in_time_zone    #=> Thu, 04 Dec 2014 18:57:28 EST -05:00
    dt + 1 == dt.since(1)             #=> true
    dt + 1.day == dt.since(1.day)     #=> true
    dt + 1.month == dt.since(1.month) #=> false

As you can see, the only case in which they differ is when the interval added to a DateTime is in a format like 1.month.

However, this usage of "since" is explicitly discouraged by the documentation of DateTime#since:

Returns a new DateTime representing the time a number of seconds since the instance time.
Do not use this method in combination with x.months, use months_since instead!

And indeed, following this recommendation the correct result is returned:

dt + 1.month == dt.months_since 1 #=> true

Therefore, my proposal is to remove the method definition of TimeWithZone#since and instead replace it with a simple alias_method :since, :+.

The rationale is that the only case where they differ is a case that is explicitly discouraged as "wrong".

In my opinion, having two methods named since and + and having to figure out exactly what the difference is makes the codebase more confusing.

However, I understand this PR is "subjective", so if you feel like it's better to ignore this, feel free to close the PR.

Thanks!

Stems from [Google group discussion](https://groups.google.com/forum/#!topic/rubyonrails-core/jSPbP-TNLb0).

Currently `AS::TimeWithZone` has two methods to add an interval to a time:
`+(other)` and `since(other)` ([docs](http://edgeapi.rubyonrails.org/classes/ActiveSupport/TimeWithZone.html)).

The two methods are "pretty much" equivalent in every case:

1. When adding any interval to an `AS::TimeWithZone` representing a `Time`:

  ```ruby
  t = Time.now.in_time_zone       #=> Thu, 04 Dec 2014 18:56:28 EST -05:00
  t + 1 == t.since(1)             #=> true
  t + 1.day == t.since(1.day)     #=> true
  t + 1.month == t.since(1.month) #=> true
  ```

2. When adding any interval to an `AS::TimeWithZone` representing a `Date`:

  ```ruby
  d = Date.today.in_time_zone     #=> Thu, 04 Dec 2014 00:00:00 EST -05:00
  d + 1 == d.since(1)             #=> true
  d + 1.day == d.since(1.day)     #=> true
  d + 1.month == d.since(1.month) #=> true
  ```

3. When adding any interval to an `AS::TimeWithZone` representing a `DateTime`:

  ```ruby
  dt = DateTime.now.in_time_zone    #=> Thu, 04 Dec 2014 18:57:28 EST -05:00
  dt + 1 == dt.since(1)             #=> true
  dt + 1.day == dt.since(1.day)     #=> true
  dt + 1.month == dt.since(1.month) #=> false
  ```

As you can see, the only case in which they differ is when the interval added
to a `DateTime` is in a format like `1.month`.

However, this usage of "since" is explicitly discouraged by the
[documentation of `DateTime#since`](https://github.com/rails/rails/blob/master/activesupport/lib/active_support/core_ext/date_time/calculations.rb#L86L88):

> Returns a new DateTime representing the time a number of seconds since the instance time.
> Do not use this method in combination with x.months, use months_since instead!

And indeed, following this recommendation the correct result is returned:

```ruby
dt + 1.month == dt.months_since 1 #=> true
```

Therefore, my proposal is to remove the method definition of `TimeWithZone#since`
and instead replace it with a simple `alias_method :since, :+`.

The rationale is that the only case where they differ is a case that is
explicitly discouraged as "wrong".

In my opinion, having two methods named `since` and `+` and having to figure
out exactly what the difference is makes the codebase more confusing.

However, I understand this PR is "subjective", so if you feel like it's better
to ignore this, feel free to close the PR.

Thanks!
rafaelfranca added a commit that referenced this pull request Dec 17, 2014
Replace AS::TimeWithZone#since with alias to +
@rafaelfranca rafaelfranca merged commit 320f75b into rails:master Dec 17, 2014
@claudiob claudiob deleted the remove-as-time-with-zone-since branch December 17, 2014 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants