Skip to content

Commit

Permalink
Use #prepend rather than using 2 aliases
Browse files Browse the repository at this point in the history
  • Loading branch information
@yuki24
yuki24 committed Apr 14, 2015
1 parent 34d3a60 commit d3684c4
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 42 deletions.
38 changes: 16 additions & 22 deletions activesupport/lib/active_support/core_ext/range/each.rb
View file
@@ -1,27 +1,21 @@
class Range #:nodoc:
module ActiveSupport
module EachTimeWithZone #:nodoc:
def each(&block)
ensure_iteration_allowed
super
end

def each_with_time_with_zone(&block)
ensure_iteration_allowed
each_without_time_with_zone(&block)
end
# TODO: change to Module#prepend as soon as the fix is backported to MRI 2.2:
# https://bugs.ruby-lang.org/issues/10847
alias_method :each_without_time_with_zone, :each
alias_method :each, :each_with_time_with_zone
def step(n = 1, &block)
ensure_iteration_allowed
super
end

def step_with_time_with_zone(n = 1, &block)
ensure_iteration_allowed
step_without_time_with_zone(n, &block)
end
# TODO: change to Module#prepend as soon as the fix is backported to MRI 2.2:
# https://bugs.ruby-lang.org/issues/10847
alias_method :step_without_time_with_zone, :step
alias_method :step, :step_with_time_with_zone
private

private
def ensure_iteration_allowed
if first.is_a?(Time)
raise TypeError, "can't iterate from #{first.class}"
end
def ensure_iteration_allowed
raise TypeError, "can't iterate from #{first.class}" if first.is_a?(Time)
end
end
end

Range.prepend(ActiveSupport::EachTimeWithZone)
40 changes: 20 additions & 20 deletions activesupport/lib/active_support/core_ext/range/include_range.rb
View file
@@ -1,23 +1,23 @@
class Range
# Extends the default Range#include? to support range comparisons.
# (1..5).include?(1..5) # => true
# (1..5).include?(2..3) # => true
# (1..5).include?(2..6) # => false
#
# The native Range#include? behavior is untouched.
# ('a'..'f').include?('c') # => true
# (5..9).include?(11) # => false
def include_with_range?(value)
if value.is_a?(::Range)
# 1...10 includes 1..9 but it does not include 1..10.
operator = exclude_end? && !value.exclude_end? ? :< : :<=
include_without_range?(value.first) && value.last.send(operator, last)
else
include_without_range?(value)
module ActiveSupport
module IncludeWithRange #:nodoc:
# Extends the default Range#include? to support range comparisons.
# (1..5).include?(1..5) # => true
# (1..5).include?(2..3) # => true
# (1..5).include?(2..6) # => false
#
# The native Range#include? behavior is untouched.
# ('a'..'f').include?('c') # => true
# (5..9).include?(11) # => false
def include?(value)
if value.is_a?(::Range)
# 1...10 includes 1..9 but it does not include 1..10.
operator = exclude_end? && !value.exclude_end? ? :< : :<=
super(value.first) && value.last.send(operator, last)

This comment has been minimized.

Sign in to view

Copy link
@baburdick

baburdick Jan 29, 2016

This logic breaks expressions like this:

case number
when   0...400            then 100.0
when 400..Float::INFINITY then 200.0
else raise StandardError
end

Why not instead feed the start and end values of the passed range to #include_without_range?

In the meantime, it's possible to workaround the breakage in my example by using Float::MAX instead:

case number
when   0...400       then 100.0
when 400..Float::MAX then 200.0
else raise StandardError
end

This comment has been minimized.

Sign in to view

Copy link
@albertosaurus

albertosaurus Feb 1, 2016

@baburdick is this what you have in mind?

def include?(value)
  if value.is_a?(::Range)
    super(value.first) && super(value.last)
  else
    super
  end
end

This comment has been minimized.

Sign in to view

Copy link
@baburdick

baburdick Feb 1, 2016

It appears that that would do it.

This comment has been minimized.

Sign in to view

Copy link
@albertosaurus

albertosaurus Feb 2, 2016

The problem with that approach is that (x...y).include?(x...y) should evaluate to true, but won't.

This comment has been minimized.

Sign in to view

Copy link
@lorefnon

lorefnon Feb 14, 2016

@baburdick In your example, for what value of number is the result unexpected ?

This comment has been minimized.

Sign in to view

Copy link
@nathell

nathell Mar 1, 2016

@lorefnon I don't see it either. Because case calls === rather than include? under the hood, it would appear that the example is not relevant. In fact, it works for me on Rails master.

This comment has been minimized.

Sign in to view

Copy link
@baburdick

baburdick Mar 1, 2016

I apologize. I was beginning to think I can no longer produce this, and that it must have been a strange interaction between a recent rails version (prior to 4.1.14.1) and a gem. But I recovered my notes on this. The problem manifests when the number is a BigDecimal. Rails console example (Rails 4.1.14.2, Ruby v2.1.2):

2.1.2 :013 > number = BigDecimal.new 1000000
 => #<BigDecimal:660fd00,'0.1E7',9(27)> 
2.1.2 :019 > number < Float::MAX
 => true 
2.1.2 :020 > number < Float::INFINITY
FloatDomainError: Infinity
    from (irb):20:in `to_r'
    from (irb):20:in `<'
    from (irb):20
    from /usr/local/rvm/gems/ruby-2.1.2@my_service/gems/railties-4.1.14.2/lib/rails/commands/console.rb:90:in `start'
    from /usr/local/rvm/gems/ruby-2.1.2@my_service/gems/railties-4.1.14.2/lib/rails/commands/console.rb:9:in `start'
    from /usr/local/rvm/gems/ruby-2.1.2@my_service/gems/railties-4.1.14.2/lib/rails/commands/commands_tasks.rb:69:in `console'
    from /usr/local/rvm/gems/ruby-2.1.2@my_service/gems/railties-4.1.14.2/lib/rails/commands/commands_tasks.rb:40:in `run_command!'
    from /usr/local/rvm/gems/ruby-2.1.2@my_service/gems/railties-4.1.14.2/lib/rails/commands.rb:17:in `<top (required)>'
    from bin/rails:4:in `require'
    from bin/rails:4:in `<main>'

Sorry to have wasted your time. With luck I'll remember to post more environment details with such reports.

This comment has been minimized.

Sign in to view

Copy link
@baburdick

baburdick Mar 1, 2016

Looks like it's this bug: https://bugs.ruby-lang.org/issues/10109

This comment has been minimized.

Sign in to view

Copy link
@baburdick

baburdick Mar 1, 2016

Looks like the most appropriate current workaround is along these lines:

(400..BigDecimal("Infinity")) === number
# => true
else
super
end
end
end
# TODO: change to Module#prepend as soon as the fix is backported to MRI 2.2:
# https://bugs.ruby-lang.org/issues/10847
alias_method :include_without_range?, :include?
alias_method :include?, :include_with_range?
end

Range.prepend(ActiveSupport::IncludeWithRange)

0 comments on commit d3684c4

Please sign in to comment.