Rails Ruby on Rails

Comments and changes to this ticket

• You flagged this item as spam.
  

  Mislav October 24th, 2009 @ 07:02 PM

  This line doesn't look right at all:

  ids.inject([]) {|m, o| m << (o.kind_of?(Range) ? o.to_a : o)}.flatten.compact.uniq
  

  And why special-case Ranges? Why not accept anything that has a to_a method?

• 

  CancelProfileIsBroken October 24th, 2009 @ 07:49 PM

  We could almost do

  ids = ids.map(&:to_a).flatten.compact.uniq
  

  The problem is that under 1.9, Fixnum doesn't have a to_a. So the code is more about special-casing Fixnum. Not clear to me that picking up everything that has to_a is the right answer - we need a specific thing, an array of integers, no?

• You flagged this item as spam.
  

  Mike Breen October 24th, 2009 @ 08:13 PM

  @Mislav - Thanks for the feedback. I guess it would be better to handle anything that has a to_a:

  ids.inject([]) {|m,o| m << (o.respond_to? :to_a ? o.to_a : o)}.flatten.compact.uniq
  

  Other than the special case for Ranges what else doesn't look right about the line?

• You flagged this item as spam.
  

  MatthewRudy October 25th, 2009 @ 09:46 AM

  in 1.9 we still get;
  

  Array(2)    # => [2]
  Array(4..6) # => [4, 5, 6]
  

  so
  

  ids.map{|id| Array(id)}.flatten.compact.uniq
  

• You flagged this item as spam.
  

  Mike Breen October 25th, 2009 @ 11:16 AM

  @MatthewRudy - that's much nicer. Thanks!

  I've attached an updated patch with Matthew's changes.

  • activerecord_find_with_range_ii.diff 2.1 KB
• 

  Matt Jones October 26th, 2009 @ 03:29 AM

  This seems like a bad idea - what about people who are using non-numeric primary keys (GUIDs, for instance)?

  I'm also not clear on what the use case for this is - is it really that much more effort to to_a the range yourself?

• You flagged this item as spam.
  

  Mike Breen October 26th, 2009 @ 01:56 PM

  @Matt Jones - Thanks for the feedback.

  This seems like a bad idea - what about people who are using non-numeric primary keys (GUIDs, for instance)?

  You're correct, this is probably not very useful for people using non-numeric primary keys. I guess my only answer is that they shouldn't use Ranges in their finds. Adding this won't change SOP for those using non-numeric primary keys and will only be a benefit to those who stick with the Rails conventions.

  I'm also not clear on what the use case for this is - is it really that much more effort to to_a the range yourself?

  That's pretty much what I do now >> Person.find(1, (4..7).to_a) and I thought that if I'm a using a Range in my find then someone else may find it useful also. But the case may be that this does not belong in Core and is only a very specific use case.

• You flagged this item as spam.
  

  Mike Breen October 26th, 2009 @ 08:50 PM

  • Tag changed from “2, 2-3-stable, active_record, find, patch” to “2-3-stable, active_record, find, patch”
• 

  Dan Pickett May 9th, 2010 @ 05:51 PM

  • Tag changed from “2-3-stable, active_record, find, patch” to “2-3-stable, active_record, bugmash, find, patch”
• 

  Rizwan Reza May 15th, 2010 @ 03:57 PM

  • Milestone set to “2.3.6”
  • State changed from “new” to “open”
• 

  Rizwan Reza May 15th, 2010 @ 04:33 PM

  +1 verified

  I have attached a patch for Rails 3 as well.

  • 0001-allow-ranges-in-finder.patch 1.5 KB
• 

  Rizwan Reza May 15th, 2010 @ 04:40 PM

  Uses Array() now.

  • 0001-allow-ranges-in-finder.patch 1.5 KB
• 

  Rizwan Reza May 15th, 2010 @ 04:50 PM

  • Milestone cleared.
  • Tag changed from “2-3-stable, active_record, bugmash, find, patch” to “2-3-stable, 3.0, active_record, bugmash, bugmash-review, find, patch”
• 

  Jeremy Kemper May 15th, 2010 @ 05:26 PM

  Need to use Array.wrap() since Array() complains about destructuring strings on 1.8.8. (require 'active_support/core_ext/array/wrap)

• 

  Rizwan Reza May 15th, 2010 @ 05:33 PM

  • Assigned user set to “Jeremy Kemper”

  Done.

  • 0001-allow-ranges-in-finder.patch 1.5 KB
• 

  Jeremy Kemper May 15th, 2010 @ 05:37 PM

  Also, is flatten needed? That could be
  

  ids.sum { |id| Array.wrap(id) }.uniq
  

• You flagged this item as spam.
  

  Mike Breen May 15th, 2010 @ 05:40 PM

  Updated the 2.3x patch with Jeremy's recommendations.

  • activerecord_find_with_range_2x.diff 2.1 KB
• 

  Rizwan Reza May 15th, 2010 @ 05:49 PM

  Mike, tests fail on this one, you will need to change it like this:

  ids = ids.map {|id| Array.wrap(id)}.flatten.compact.uniq

• 

  Jeremy Kemper May 15th, 2010 @ 05:52 PM

  Reading back up, this means find(1, 5..10000) will generate a query with 9996 ids. This should use normal range query mapping instead.

• 

  Jeremy Kemper May 15th, 2010 @ 05:52 PM

  • State changed from “open” to “wontfix”
• 

  Rizwan Reza May 15th, 2010 @ 06:36 PM

  • Tag changed from “2-3-stable, 3.0, active_record, bugmash, bugmash-review, find, patch” to “2-3-stable, 3.0, active_record, find, patch”