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 Enumerable#pluck. #20350

Merged
merged 1 commit into from May 29, 2015

Conversation

Projects
None yet
9 participants
@kddeisz
Contributor

kddeisz commented May 29, 2015

Allows fetching the same values from arrays as from ActiveRecord associations.

As requested by DHH in #20339.

Add Enumerable#pluck.
Allows fetching the same values from arrays as from ActiveRecord associations.

@kddeisz kddeisz referenced this pull request May 29, 2015

Closed

Add Enumerable#pluck #20339

@kddeisz kddeisz closed this May 29, 2015

@kddeisz kddeisz reopened this May 29, 2015

rafaelfranca added a commit that referenced this pull request May 29, 2015

@rafaelfranca rafaelfranca merged commit 57f51f0 into rails:master May 29, 2015

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca May 29, 2015

Member

Awesome! Thank you.

Member

rafaelfranca commented May 29, 2015

Awesome! Thank you.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca May 29, 2015

Member

Oh. I just forgot about the guides. Could you update the ative support guide?

Member

rafaelfranca commented May 29, 2015

Oh. I just forgot about the guides. Could you update the ative support guide?

@kddeisz

This comment has been minimized.

Show comment
Hide comment
@kddeisz

kddeisz May 29, 2015

Contributor

Yeah just one sec

Contributor

kddeisz commented May 29, 2015

Yeah just one sec

@kddeisz

This comment has been minimized.

Show comment
Hide comment
@kddeisz

kddeisz May 29, 2015

Contributor

It's over there: #20351

Contributor

kddeisz commented May 29, 2015

It's over there: #20351

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh May 29, 2015

Member

Lovely! Would be great to make AR use this method if the scope has already been loaded as well.

Member

dhh commented May 29, 2015

Lovely! Would be great to make AR use this method if the scope has already been loaded as well.

@bogdan

This comment has been minimized.

Show comment
Hide comment
@bogdan

bogdan May 29, 2015

Contributor

Wow, not so fast plz

There are some problems:

AR pluck supports multiple arguments, but this one doesn't.

I am not sure why [] is used here instead of send, but it will not support non-AR objects in Enum that doesn't have [] working similar to send.

Sometimes User.all.to_a.pluck(:email) can produce different result from User.all.pluck(:email).
This is something I probably can live with, but when we talk about loaded associations the problem gets worse: user.projects.pluck(:name) can produce different results when association is loaded or not if name is modified in object after loading.

These 3 things are not so critical concerns but we will definitely hear about them sometimes in bug reports.

Contributor

bogdan commented May 29, 2015

Wow, not so fast plz

There are some problems:

AR pluck supports multiple arguments, but this one doesn't.

I am not sure why [] is used here instead of send, but it will not support non-AR objects in Enum that doesn't have [] working similar to send.

Sometimes User.all.to_a.pluck(:email) can produce different result from User.all.pluck(:email).
This is something I probably can live with, but when we talk about loaded associations the problem gets worse: user.projects.pluck(:name) can produce different results when association is loaded or not if name is modified in object after loading.

These 3 things are not so critical concerns but we will definitely hear about them sometimes in bug reports.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh May 29, 2015

Member

Actually, it would be nice if we could make Enumerable#pluck support
multiple arguments. Should be just as easy.

We're using #[] explicitly to get around overwritten methods. So if you
have def name() "The Honorable #{self[:name}" end defined,
unloaded_scope.pluck(:name) and loaded_scope.pluck(:name) would return two
different things.

I think it's good to document these potential gotchas, though.

On Fri, May 29, 2015 at 1:58 PM, Bogdan Gusiev notifications@github.com
wrote:

Wow, not so fast plz

There are some problems:

AR pluck supports multiple arguments, but this one doesn't.

I am not sure why [] is used here instead of send, but it will not
support non-AR objects in Enum that doesn't have [] working similar to
send.

Sometimes User.all.to_a.pluck(:email) can produce different result from
User.all.pluck(:email).
This is something I probably can live with, but when we talk about loaded
associations the problem gets worse: user.projects.pluck(:name) can
produce different results when association is loaded or not if name is
modified in object after loading.

These 3 things are not so critical concerns but we will definitely hear
about them sometimes in bug reports.


Reply to this email directly or view it on GitHub
#20350 (comment).

Member

dhh commented May 29, 2015

Actually, it would be nice if we could make Enumerable#pluck support
multiple arguments. Should be just as easy.

We're using #[] explicitly to get around overwritten methods. So if you
have def name() "The Honorable #{self[:name}" end defined,
unloaded_scope.pluck(:name) and loaded_scope.pluck(:name) would return two
different things.

I think it's good to document these potential gotchas, though.

On Fri, May 29, 2015 at 1:58 PM, Bogdan Gusiev notifications@github.com
wrote:

Wow, not so fast plz

There are some problems:

AR pluck supports multiple arguments, but this one doesn't.

I am not sure why [] is used here instead of send, but it will not
support non-AR objects in Enum that doesn't have [] working similar to
send.

Sometimes User.all.to_a.pluck(:email) can produce different result from
User.all.pluck(:email).
This is something I probably can live with, but when we talk about loaded
associations the problem gets worse: user.projects.pluck(:name) can
produce different results when association is loaded or not if name is
modified in object after loading.

These 3 things are not so critical concerns but we will definitely hear
about them sometimes in bug reports.


Reply to this email directly or view it on GitHub
#20350 (comment).

@bogdan

This comment has been minimized.

Show comment
Hide comment
@bogdan

bogdan May 29, 2015

Contributor

Yes, it is cool that we've protected from def name... but there is after_intialize where there could be defaults for attributes. The reasons why they are not in DB can be different from no-reason to default value is dynamic and based on conditions.

Contributor

bogdan commented May 29, 2015

Yes, it is cool that we've protected from def name... but there is after_intialize where there could be defaults for attributes. The reasons why they are not in DB can be different from no-reason to default value is dynamic and based on conditions.

@kddeisz

This comment has been minimized.

Show comment
Hide comment
@kddeisz

kddeisz May 29, 2015

Contributor

That's the issue exactly I think - you could have after_initialize, after_find, def name, or number of other potential modifiers to the name attribute. I think we have to go with #[].

That being said, this feels a lot like #size, which will change depending on loaded or unloaded, and could produce different results if you put none persisted ARs into the association. As long as it's documented it seems alright.

Contributor

kddeisz commented May 29, 2015

That's the issue exactly I think - you could have after_initialize, after_find, def name, or number of other potential modifiers to the name attribute. I think we have to go with #[].

That being said, this feels a lot like #size, which will change depending on loaded or unloaded, and could produce different results if you put none persisted ARs into the association. As long as it's documented it seems alright.

@renanmedina

This comment has been minimized.

Show comment
Hide comment
@renanmedina

renanmedina May 29, 2015

Nice man !!!

Nice man !!!

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh May 29, 2015

Member

Agree. We do best effort approximation and document the corner cases.

On Fri, May 29, 2015 at 2:13 PM, Kevin Deisz notifications@github.com
wrote:

That's the issue exactly I think - you could have after_initialize,
after_find, def name, or number of other potential modifiers to the name
attribute. I think we have to go with #[].

That being said, this feels a lot like #size, which will change depending
on loaded or unloaded, and could produce different results if you put none
persisted ARs into the association. As long as it's documented it seems
alright.


Reply to this email directly or view it on GitHub
#20350 (comment).

Member

dhh commented May 29, 2015

Agree. We do best effort approximation and document the corner cases.

On Fri, May 29, 2015 at 2:13 PM, Kevin Deisz notifications@github.com
wrote:

That's the issue exactly I think - you could have after_initialize,
after_find, def name, or number of other potential modifiers to the name
attribute. I think we have to go with #[].

That being said, this feels a lot like #size, which will change depending
on loaded or unloaded, and could produce different results if you put none
persisted ARs into the association. As long as it's documented it seems
alright.


Reply to this email directly or view it on GitHub
#20350 (comment).

@kddeisz

This comment has been minimized.

Show comment
Hide comment
@kddeisz

kddeisz May 29, 2015

Contributor

Will work on this today

Contributor

kddeisz commented May 29, 2015

Will work on this today

@kddeisz

This comment has been minimized.

Show comment
Hide comment
@kddeisz

kddeisz May 29, 2015

Contributor

Pull request: #20362

Contributor

kddeisz commented May 29, 2015

Pull request: #20362

@uberllama

This comment has been minimized.

Show comment
Hide comment
@uberllama

uberllama Jun 3, 2015

Contributor

This seems really strange to me.

In the very same rubyonrails.org post that this feature was announced, there is a deprecation notice for .uniq on Relation for the following reason:

The superficial similarity between Relation#uniq and Array#uniq has been a source of confusion, which led to the addition of Relation#distinct which better communicates what is happening under the hood.

And yet here we are adding a method to Enumerable to make it more similar to Relation? Which direction are we going in our APIs, uniformity or specificity?

Contributor

uberllama commented Jun 3, 2015

This seems really strange to me.

In the very same rubyonrails.org post that this feature was announced, there is a deprecation notice for .uniq on Relation for the following reason:

The superficial similarity between Relation#uniq and Array#uniq has been a source of confusion, which led to the addition of Relation#distinct which better communicates what is happening under the hood.

And yet here we are adding a method to Enumerable to make it more similar to Relation? Which direction are we going in our APIs, uniformity or specificity?

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jun 3, 2015

Member

We're weighing every case on its own merits. Consistency for its own sake
is the hobgoblin and all that...

On Wed, Jun 3, 2015 at 9:39 PM, Yuval Kordov notifications@github.com
wrote:

This seems really strange to me.

In the very same rubyonrails.org post that this feature was announced,
there is a deprecation notice for .uniq on Relation for the following
reason:

The superficial similarity between Relation#uniq and Array#uniq has been a
source of confusion, which led to the addition of Relation#distinct which
better communicates what is happening under the hood.

And yet here we are adding a method to Array to make it more similar to
Relation? Which direction are we going in our APIs, uniformity or
specificity?


Reply to this email directly or view it on GitHub
#20350 (comment).

Member

dhh commented Jun 3, 2015

We're weighing every case on its own merits. Consistency for its own sake
is the hobgoblin and all that...

On Wed, Jun 3, 2015 at 9:39 PM, Yuval Kordov notifications@github.com
wrote:

This seems really strange to me.

In the very same rubyonrails.org post that this feature was announced,
there is a deprecation notice for .uniq on Relation for the following
reason:

The superficial similarity between Relation#uniq and Array#uniq has been a
source of confusion, which led to the addition of Relation#distinct which
better communicates what is happening under the hood.

And yet here we are adding a method to Array to make it more similar to
Relation? Which direction are we going in our APIs, uniformity or
specificity?


Reply to this email directly or view it on GitHub
#20350 (comment).

@bogdan

This comment has been minimized.

Show comment
Hide comment
@bogdan

bogdan Jun 4, 2015

Contributor

I agree with @uberllama. This is magic feature that can cause more problems than solutions.
I didn't feel a lot of pain when wrote the following in my project:

  def tag_names
    # Performance optimisation:
    tags.loaded? ? tags.map(&:name) : tags.pluck(:name)
  end

While the problem we have solved is pretty rare: only one place where it is useful in 5 years old project.

I heard many times before things like "No, we will not accept this PR because Relation is not Array" in features that solved more than one of my problems: https://github.com/rails/rails/pull/8794/files

So I am still 👎 on this feature.

Contributor

bogdan commented Jun 4, 2015

I agree with @uberllama. This is magic feature that can cause more problems than solutions.
I didn't feel a lot of pain when wrote the following in my project:

  def tag_names
    # Performance optimisation:
    tags.loaded? ? tags.map(&:name) : tags.pluck(:name)
  end

While the problem we have solved is pretty rare: only one place where it is useful in 5 years old project.

I heard many times before things like "No, we will not accept this PR because Relation is not Array" in features that solved more than one of my problems: https://github.com/rails/rails/pull/8794/files

So I am still 👎 on this feature.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jun 4, 2015

Member

Objection noted. Thanks for your feedback. I consider that tag_names
implementation to be pretty nasty, personally. Plenty of precedence here,
like relation#size etc.

On Thu, Jun 4, 2015 at 1:12 PM, Bogdan Gusiev notifications@github.com
wrote:

I agree with @uberllama https://github.com/uberllama. This is magic
feature that can cause more problems than solutions.
I didn't feel a lot of pain when wrote the following in my project:

def tag_names
# Performance optimisation:
tags.loaded? ? tags.map(&:name) : tags.pluck(:name)
end

While the problem we have solved is pretty rare: only one place where it
is useful in 5 years old project.

I heard many times before things like "No, we will not accept this PR
because Relation is not Array" in features that solved more than one of my
problems: https://github.com/rails/rails/pull/8794/files

So I am still [image: 👎] on this feature.


Reply to this email directly or view it on GitHub
#20350 (comment).

Member

dhh commented Jun 4, 2015

Objection noted. Thanks for your feedback. I consider that tag_names
implementation to be pretty nasty, personally. Plenty of precedence here,
like relation#size etc.

On Thu, Jun 4, 2015 at 1:12 PM, Bogdan Gusiev notifications@github.com
wrote:

I agree with @uberllama https://github.com/uberllama. This is magic
feature that can cause more problems than solutions.
I didn't feel a lot of pain when wrote the following in my project:

def tag_names
# Performance optimisation:
tags.loaded? ? tags.map(&:name) : tags.pluck(:name)
end

While the problem we have solved is pretty rare: only one place where it
is useful in 5 years old project.

I heard many times before things like "No, we will not accept this PR
because Relation is not Array" in features that solved more than one of my
problems: https://github.com/rails/rails/pull/8794/files

So I am still [image: 👎] on this feature.


Reply to this email directly or view it on GitHub
#20350 (comment).

@uberllama

This comment has been minimized.

Show comment
Hide comment
@uberllama

uberllama Jun 4, 2015

Contributor

What's funny is I actually like API uniformity. So my comment was more in context of disagreeing with uniq being removed from Relation than pluck being added to Enumerable. I don't think there's anything goblinesque about consistency. 😈

Contributor

uberllama commented Jun 4, 2015

What's funny is I actually like API uniformity. So my comment was more in context of disagreeing with uniq being removed from Relation than pluck being added to Enumerable. I don't think there's anything goblinesque about consistency. 😈

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jun 4, 2015

Member

I haven't dug in closer to the #uniq case. Will have another look at that.
But I don't think just because we can make reasonable trade-offs in one
case, like #pluck, that we necessarily can too in all other like-array
cases.

On Thu, Jun 4, 2015 at 3:59 PM, Yuval Kordov notifications@github.com
wrote:

What's funny is I actually like API uniformity. So my comment was more in
context of disagreeing with uniq being removed from Relation than pluck
being added to Enumerable. I don't think there's anything goblinesque about
consistency. [image: 😈]


Reply to this email directly or view it on GitHub
#20350 (comment).

Member

dhh commented Jun 4, 2015

I haven't dug in closer to the #uniq case. Will have another look at that.
But I don't think just because we can make reasonable trade-offs in one
case, like #pluck, that we necessarily can too in all other like-array
cases.

On Thu, Jun 4, 2015 at 3:59 PM, Yuval Kordov notifications@github.com
wrote:

What's funny is I actually like API uniformity. So my comment was more in
context of disagreeing with uniq being removed from Relation than pluck
being added to Enumerable. I don't think there's anything goblinesque about
consistency. [image: 😈]


Reply to this email directly or view it on GitHub
#20350 (comment).

@kddeisz

This comment has been minimized.

Show comment
Hide comment
@kddeisz

kddeisz Jun 4, 2015

Contributor

It's actually more uniformity, because a lot of the relation/array duplications are also in calculations.rb. So there's that.

Contributor

kddeisz commented Jun 4, 2015

It's actually more uniformity, because a lot of the relation/array duplications are also in calculations.rb. So there's that.

@onomojo

This comment has been minimized.

Show comment
Hide comment
@onomojo

onomojo Jun 5, 2015

Would be nice to allow the same syntax for multi column support that AR has

onomojo commented Jun 5, 2015

Would be nice to allow the same syntax for multi column support that AR has

@kddeisz

This comment has been minimized.

Show comment
Hide comment
@kddeisz

kddeisz Jun 5, 2015

Contributor

@onomojo that was the next step here: #20362

Contributor

kddeisz commented Jun 5, 2015

@onomojo that was the next step here: #20362

@onomojo

This comment has been minimized.

Show comment
Hide comment
@onomojo

onomojo Jun 5, 2015

Ah ok, I was just looking at the PR. 👍

onomojo commented Jun 5, 2015

Ah ok, I was just looking at the PR. 👍

@WizardOfOgz

This comment has been minimized.

Show comment
Hide comment
@WizardOfOgz

WizardOfOgz Jun 17, 2015

I'm late to the party, but I have concerns with this addition. It seems to focus only on Hashs and plucking using hash keys. Should this not be implemented as Hash#pluck instead of Enumerable#pluck?

UPDATE: NM I didn't think this through clearly

I'm late to the party, but I have concerns with this addition. It seems to focus only on Hashs and plucking using hash keys. Should this not be implemented as Hash#pluck instead of Enumerable#pluck?

UPDATE: NM I didn't think this through clearly

@kddeisz

This comment has been minimized.

Show comment
Hide comment
@kddeisz

kddeisz Jun 17, 2015

Contributor

Plenty of things respond to [] that are both Enumerable and not hashes.
E.g. AR objects

On Wednesday, June 17, 2015, Andy Ogzewalla notifications@github.com
wrote:

I'm late to the party, but I have concerns with this addition. It seems to
focus only on Hashs and plucking using hash keys. Should this not be
implemented as Hash#pluck instead of Enumerable#pluck?


Reply to this email directly or view it on GitHub
#20350 (comment).

Kevin D. Deisz
TrialNetworks - part of DrugDev
Software Developer
383 Elliot Street, Suite G
Newton, MA 02464
+1 617.952.4071 x134 (office)
+1 703.615.0396 (mobile)
kdeisz@trialnetworks.com

Contributor

kddeisz commented Jun 17, 2015

Plenty of things respond to [] that are both Enumerable and not hashes.
E.g. AR objects

On Wednesday, June 17, 2015, Andy Ogzewalla notifications@github.com
wrote:

I'm late to the party, but I have concerns with this addition. It seems to
focus only on Hashs and plucking using hash keys. Should this not be
implemented as Hash#pluck instead of Enumerable#pluck?


Reply to this email directly or view it on GitHub
#20350 (comment).

Kevin D. Deisz
TrialNetworks - part of DrugDev
Software Developer
383 Elliot Street, Suite G
Newton, MA 02464
+1 617.952.4071 x134 (office)
+1 703.615.0396 (mobile)
kdeisz@trialnetworks.com

@WizardOfOgz

This comment has been minimized.

Show comment
Hide comment
@WizardOfOgz

WizardOfOgz Jun 17, 2015

NM, I wasn't thinking straight

NM, I wasn't thinking straight

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