Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

refactored Time#<=> and DateTime#<=> by removing unnecessary calls wi…

…thout losing performance

Signed-off-by: Santiago Pastorino <santiago@wyeworks.com>
  • Loading branch information...
commit 7872cc992bac76e976334d0c20649d69aad5652e 1 parent 0db915e
@dcrec1 dcrec1 authored spastorino committed
View
10 activesupport/lib/active_support/core_ext/date_time/calculations.rb
@@ -1,6 +1,4 @@
require 'rational' unless RUBY_VERSION >= '1.9.2'
-require 'active_support/core_ext/object/acts_like'
-require 'active_support/core_ext/time/zones'
class DateTime
class << self
@@ -105,11 +103,7 @@ def utc_offset
end
# Layers additional behavior on DateTime#<=> so that Time and ActiveSupport::TimeWithZone instances can be compared with a DateTime
- def compare_with_coercion(other)
- other = other.comparable_time if other.respond_to?(:comparable_time)
- other = other.to_datetime unless other.acts_like?(:date)
- compare_without_coercion(other)
+ def <=>(other)
+ super other.to_datetime
end
- alias_method :compare_without_coercion, :<=>
- alias_method :<=>, :compare_with_coercion
end
View
13 activesupport/lib/active_support/core_ext/time/calculations.rb
@@ -1,7 +1,4 @@
require 'active_support/duration'
-require 'active_support/core_ext/date/acts_like'
-require 'active_support/core_ext/date/calculations'
-require 'active_support/core_ext/date_time/conversions'
class Time
COMMON_YEAR_DAYS_IN_MONTH = [nil, 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31]
@@ -283,14 +280,8 @@ def minus_with_coercion(other)
# Layers additional behavior on Time#<=> so that DateTime and ActiveSupport::TimeWithZone instances
# can be chronologically compared with a Time
def compare_with_coercion(other)
- # if other is an ActiveSupport::TimeWithZone, coerce a Time instance from it so we can do <=> comparison
- other = other.comparable_time if other.respond_to?(:comparable_time)
- if other.acts_like?(:date)
- # other is a Date/DateTime, so coerce self #to_datetime and hand off to DateTime#<=>
- to_datetime.compare_without_coercion(other)
- else
- compare_without_coercion(other)
- end
+ # we're avoiding Time#to_datetime cause it's expensive
+ other.is_a?(Time) ? compare_without_coercion(other.to_time) : to_datetime <=> other
end
alias_method :compare_without_coercion, :<=>
alias_method :<=>, :compare_with_coercion
View
2  activesupport/lib/active_support/time_with_zone.rb
@@ -281,7 +281,7 @@ def to_i
# A TimeWithZone acts like a Time, so just return +self+.
def to_time
- self
+ utc
@tiegz
tiegz added a note

Hello, couldn't find the pull request for this one, but was wondering what the reasoning behind this change was. Seems like you might not always want UTC when expecting self otherwise (if self is not UTC).

@steveklabnik Collaborator

Posting this question to rails-core is much more likely to get an answer than a comment on a commit.

@tiegz
tiegz added a note

Thx for the suggestion. Thought it might be the perfect place since it's contextual, but now that you mention that I realize I'm probably polluting core members' notifications :\

@steveklabnik Collaborator

It's not even that it's polluting... notifications image

When that is happening, it's just not effective.

Oh, and providing a link to this commit in the email will also provide equal context.

@tiegz
tiegz added a note

Haha, yeah, I love github but the message notifications feature needs some tlc. Thx

I, too, would like to know why this change was made.

@korny
korny added a note

For what it's worth, this change breaks things. For one thing:

DateTime.now.in_time_zone(Time.zone).to_time.class
 => DateTime 

That's wrong, it's supposed to return a Time instance.

The other thing is that this completely breaks comparisons of Time to TimeWithZone:

Time.now <=> DateTime.now  #=> -1 
Time.now <=> ActiveSupport::TimeWithZone.new(DateTime.now, Time.zone)  #=> nil
ActiveSupport::TimeWithZone.new(DateTime.now, Time.zone) <=> Time.now  #=> -1

Whatever the intention was, it was a bad commit.

@korny
korny added a note

Here's the Pull Request that eventually fixed this: #2453

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
end
def to_datetime
@tiegz

Hello, couldn't find the pull request for this one, but was wondering what the reasoning behind this change was. Seems like you might not always want UTC when expecting self otherwise (if self is not UTC).

@steveklabnik

Posting this question to rails-core is much more likely to get an answer than a comment on a commit.

@tiegz

Thx for the suggestion. Thought it might be the perfect place since it's contextual, but now that you mention that I realize I'm probably polluting core members' notifications :\

@steveklabnik

It's not even that it's polluting... notifications image

When that is happening, it's just not effective.

Oh, and providing a link to this commit in the email will also provide equal context.

@tiegz

Haha, yeah, I love github but the message notifications feature needs some tlc. Thx

@fredbenenson

I, too, would like to know why this change was made.

@korny

For what it's worth, this change breaks things. For one thing:

DateTime.now.in_time_zone(Time.zone).to_time.class
 => DateTime 

That's wrong, it's supposed to return a Time instance.

The other thing is that this completely breaks comparisons of Time to TimeWithZone:

Time.now <=> DateTime.now  #=> -1 
Time.now <=> ActiveSupport::TimeWithZone.new(DateTime.now, Time.zone)  #=> nil
ActiveSupport::TimeWithZone.new(DateTime.now, Time.zone) <=> Time.now  #=> -1

Whatever the intention was, it was a bad commit.

@korny

Here's the Pull Request that eventually fixed this: #2453

Please sign in to comment.
Something went wrong with that request. Please try again.