Model.select takes a variable list of arguments #7069

Closed
wants to merge 1 commit into
from

Projects

None yet

6 participants

@isaacsanders
Contributor

This is a cleaner version of #6916 and #6162

This is a change from the behavior of passing in an Array of args.
This solution attempts to answer issue #3165

@pixeltrix
Member

As mentioned by @josevalim in #6162, *args was removed by @tenderlove in b8899bb, so we need a clarification on why it was removed before merging.

@pixeltrix pixeltrix commented on an outdated diff Jul 16, 2012
activerecord/lib/active_record/relation/query_methods.rb
end
end
- def select!(value)
- self.select_values += Array.wrap(value)
+ def select!(*fields)
+ fields.each do |field|
+ self.select_values += Array.wrap(field)
+ end
@pixeltrix
pixeltrix Jul 16, 2012 Member

A couple of points:

  1. Isn't flatten a better solution, e.g: self.select_values += fields.flatten
  2. Isn't Array.wrap no longer required on 1.9.3 - though this is moot if 1 is true
@frodsan
Contributor
frodsan commented Aug 1, 2012

It'll be nice if this discussion gets solved, there are 2 open pull requests and 1 issue from 10 months ago.

@rafaelfranca
Member

Could you raise this discussion tomorrow with the Rails core members?

@isaacsanders
Contributor

Hey @tenderlove + @josevalim. Thoughts?

@frodsan
Contributor
frodsan commented Aug 1, 2012

@rafaelfranca Did you mean the Resque core members? haha no problem, bro.

@rafaelfranca
Member

lol

@tenderlove tenderlove and 2 others commented on an outdated diff Aug 7, 2012
activerecord/lib/active_record/relation/query_methods.rb
end
end
# Like #select, but modifies relation in place.
- def select!(value)
- self.select_values += Array.wrap(value)
+ def select!(*fields)
+ (self.select_values += fields).flatten
@tenderlove
tenderlove Aug 7, 2012 Member

The return value of flatten isn't used here. Is it necessary?

@frodsan
frodsan Aug 7, 2012 Contributor

I guess it should be:

self.select_values += fields.flatten
@isaacsanders
isaacsanders Aug 7, 2012 Contributor

True.

@tenderlove
Member

I'm hesitant to support *args as parameters to a method. It makes changing the parameters a method accepts a PITA. e.g. if we want to accept something besides column names, like maybe an ARel AST or something, we'll have to start manually picking apart the params in the *args.

@pixeltrix I just removed it because we weren't using it. I'm actually OK adding it back as a new feature if we have tests. :-)

@isaacsanders Isaac Sanders Model.select takes a variable list of arguments.
  This is a change from the behavior of passing in an Array of args.
  This solution attempts to answer issue #3165
6cf0388
@frodsan frodsan commented on the diff Aug 7, 2012
activerecord/lib/active_record/relation/query_methods.rb
else
- spawn.select!(value)
+ raise ArgumentError, 'Call this with at least one field' if fields.empty?
+ clone.select!(*fields)
@frodsan
frodsan Aug 7, 2012 Contributor

@isaacsanders Why do you use clone instead of spawn? I saw that all the methods are using spawn.

@carlosantoniodasilva
carlosantoniodasilva Aug 11, 2012 Member

Agreed with @frodsan, should stick with spawn since it's different in collection proxy.

@isaacsanders
Contributor

This seems to be somewhere else... I will close this.

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