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

Add Relation#pick as short-hand for single-value plucks #31941

Merged
merged 2 commits into from Feb 9, 2018

Conversation

Projects
None yet
6 participants
@dhh
Member

dhh commented Feb 9, 2018

Pick the first value from the named column in the current relation. This is short-hand for relation.limit(1).pluck(column_name).first, and is primarily useful when you have a relation that's already narrowed down to a single row.

Just like #pluck, #pick will only load the actual value, not the entire record object, so it's also more efficient. The value is, again like with pluck, typecast by the column type.

  Person.where(id: 1).pick(:name)
  # SELECT people.name FROM people WHERE id = 1 LIMIT 1
  # => 'David'
@composerinteralia

This comment has been minimized.

Show comment
Hide comment
@composerinteralia

composerinteralia Feb 9, 2018

Member

Might we want pick to take multiple arguments like pluck?

  Person.where(id: 1).pick(:name, :email)
  # SELECT people.name, people.email FROM people WHERE id = 1 LIMIT 1
  # => ['Daniel', 'i@heart.rails']

(I was unreasonably nervous to leave this comment. I actually wrote it, then deleted it, then wrote it again.)

Member

composerinteralia commented Feb 9, 2018

Might we want pick to take multiple arguments like pluck?

  Person.where(id: 1).pick(:name, :email)
  # SELECT people.name, people.email FROM people WHERE id = 1 LIMIT 1
  # => ['Daniel', 'i@heart.rails']

(I was unreasonably nervous to leave this comment. I actually wrote it, then deleted it, then wrote it again.)

@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Feb 9, 2018

Member

is primarily useful when you have a relation that's already narrowed down to a single row

This reminds me of some of the philosophy behind #26206. Might it be worth codifying that rule?

def pick(*column_names)
  rows = limit(2).pluck(*column_names)
  raise "Too many rows" if rows.size > 1
  rows.first
end
Member

matthewd commented Feb 9, 2018

is primarily useful when you have a relation that's already narrowed down to a single row

This reminds me of some of the philosophy behind #26206. Might it be worth codifying that rule?

def pick(*column_names)
  rows = limit(2).pluck(*column_names)
  raise "Too many rows" if rows.size > 1
  rows.first
end
@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Feb 9, 2018

Member

The specific relation that I extracted #pick from actually does comprise multiple rows, but I know that I just want to deal with the first one (because I ordered the relation in such a way. Here's the actual source:

module Person::Creator
  extend ActiveSupport::Concern

  def last_created_at_from(recordings)
    recordings.where(creator: self).reverse_chronologically.pick(:created_at)
  end
end

So a guard clause against multiple rows wouldn't really work for this. I don't think it's necessary in any case.

Member

dhh commented Feb 9, 2018

The specific relation that I extracted #pick from actually does comprise multiple rows, but I know that I just want to deal with the first one (because I ordered the relation in such a way. Here's the actual source:

module Person::Creator
  extend ActiveSupport::Concern

  def last_created_at_from(recordings)
    recordings.where(creator: self).reverse_chronologically.pick(:created_at)
  end
end

So a guard clause against multiple rows wouldn't really work for this. I don't think it's necessary in any case.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Feb 9, 2018

Member

@composerinteralia Totally. Just added that 👌

@kamipo I just changed the test for now 🙏

Member

dhh commented Feb 9, 2018

@composerinteralia Totally. Just added that 👌

@kamipo I just changed the test for now 🙏

@dhh dhh merged commit 80cc0d3 into master Feb 9, 2018

1 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details
codeclimate All good!
Details
@bogdan

This comment has been minimized.

Show comment
Hide comment
@bogdan

bogdan Feb 10, 2018

Contributor

Will it raise NoMethodError for NullRelation?

Contributor

bogdan commented Feb 10, 2018

Will it raise NoMethodError for NullRelation?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Feb 12, 2018

Member

@bogdan no. Is it raising? I just added 4c615a5 and that test is passing. As it is the only way to generate NullRelation I can't see how it would be raising.

Member

rafaelfranca commented Feb 12, 2018

@bogdan no. Is it raising? I just added 4c615a5 and that test is passing. As it is the only way to generate NullRelation I can't see how it would be raising.

@bogdan

This comment has been minimized.

Show comment
Hide comment
@bogdan

bogdan Feb 13, 2018

Contributor

@rafaelfranca It was only my theoretical guess. I think the test is good idea anyway. Thanks!

Contributor

bogdan commented Feb 13, 2018

@rafaelfranca It was only my theoretical guess. I think the test is good idea anyway. Thanks!

kamipo added a commit that referenced this pull request Feb 18, 2018

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