Skip to content

Add Enumerable#pick#38760

Merged
kaspth merged 2 commits into
rails:masterfrom
eugeneius:enumerable_pick
Mar 18, 2020
Merged

Add Enumerable#pick#38760
kaspth merged 2 commits into
rails:masterfrom
eugeneius:enumerable_pick

Conversation

@eugeneius

Copy link
Copy Markdown
Member

As with pluck (#20339), it's useful to be able to call pick on loaded relations, unloaded relations, and plain enumerables interchangeably.

This also ensures that it's always safe to rewrite pluck(:title).first as pick(:title).

Comment thread activesupport/lib/active_support/core_ext/enumerable.rb Outdated

@simi simi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok to me since it follows same pattern as pluck and there's probably no chance to get this to Ruby itself, since it is not useful for Enumerable in general.

👍

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we normalize to return an empty array like pluck does? Could potentially tap compact! at the end of the map.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can use compact, since we need to keep any nil values that were picked when the enumerable isn't empty. We could add a special case for when it is empty, though.

Using the rubric that pick(...) should be equivalent to pluck(...).first, picking an empty enumerable seems like it should return nil:

irb(main):001:0> [].pluck(:dollar, :cents).first
=> nil

This is how Relation#pick already behaves:

assert_nil Topic.none.pick(:author_name, :author_email_address)

I have a slight preference for aligning with that existing behaviour, although in practice I think the result of calling pick with multiple arguments is usually destructured, so it doesn't matter too much which behaviour we pick choose:

irb(main):001:0> a, b = nil; [a, b]
=> [nil, nil]
irb(main):002:0> a, b = []; [a, b]
=> [nil, nil]
irb(main):003:0> a, b = [nil, nil]; [a, b]
=> [nil, nil]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My objection was more to seeing [] for the pluck return versus [ nil, nil ] for pick, size would be different or similar.

I have a slight preference for aligning with that existing behaviour,

Same, but the nil is the default return for a method call, so it would do that regardless. I like the none? fix.

@eugeneius eugeneius force-pushed the enumerable_pick branch 2 times, most recently from 9ea8fd4 to 72dfcce Compare March 18, 2020 18:28
This allows `pick` to be called on an object that could either be an
enumerable or a relation.

Also clarify the documentation for `Enumerable#pluck`, and add an
example of plucking multiple keys to the core extensions guide.
When called on a loaded relation, `pick` will now use the existing
results instead of making another query, just like `pluck` does.
@kaspth kaspth merged commit 5c188c1 into rails:master Mar 18, 2020
@kaspth

kaspth commented Mar 18, 2020

Copy link
Copy Markdown
Contributor

These ain't slim pickings, bud! Thanks for adding this, @eugeneius ✌️

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants