Skip to content
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

[ci skip] Pluck loads the records but omits attributes #44004

Merged
merged 1 commit into from Dec 28, 2021

Conversation

AdityaBhutani
Copy link
Contributor

Summary

Pluck Doc incorrectly states "Use #pluck as a shortcut to select one or more attributes without loading a bunch of records just to grab the attributes you want."

Instead It loads the rows but omits the extra attributes.

Updating the doc to be consistent with functionality of pluck.

@ghiculescu
Copy link
Member

Hmm, what is incorrect about the statement? I think it’s written relative to loading all the records and then calling map to just get the attributes you want.

@AdityaBhutani
Copy link
Contributor Author

AdityaBhutani commented Dec 27, 2021

Hey @ghiculescu. It says, "without loading bunch of Records" but instead it loads the respective records, only with selective attributes.

@ghiculescu
Copy link
Member

I think "record" means initiatlising the Active Record objects. The rows are loaded from the database and they are type cast, but no AR objects are constructed. This is one of the things that makes pluck faster.

@AdityaBhutani
Copy link
Contributor Author

Thanks. This makes sense now.
Do you suggest to make it more clear by restating it like "Use #pluck as a shortcut to select one or more attributes without loading a bunch of ActiveRecord objects just to grab the attributes you want."
Reason: People have earlier been confused by this here

@ghiculescu
Copy link
Member

👍 yeah, I like that.

@AdityaBhutani
Copy link
Contributor Author

Hey @ghiculescu, I had a concern related to test cases. There are many places where we have used map(&:id) where we should use pluck(:id) or ids. Would this change be considered superficial ?

@@ -155,7 +155,7 @@ def calculate(operation, column_name)
end

# Use #pluck as a shortcut to select one or more attributes without
# loading a bunch of records just to grab the attributes you want.
# loading a bunch of ActiveRecord objects just to grab the attributes you want.
Copy link
Member

Choose a reason for hiding this comment

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

"Active Record" needs a space. But, looking at the pick docs, what do you think about this alternative:

Suggested change
# loading a bunch of ActiveRecord objects just to grab the attributes you want.
# loading an entire record object per row.

Or even this one:

Suggested change
# loading a bunch of ActiveRecord objects just to grab the attributes you want.
# instantiating a model object per row.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Second one looks perfect. Do you think we can use Active Record instead of model, because we also have Active Model.

Copy link
Member

Choose a reason for hiding this comment

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

Here, "model" refers to the "model" in model-view-controller, not Active Model.

Conversely, "Active Record" is a framework which defines many objects / classes, ActiveRecord::Base among them.

All in all, if you would prefer not to use the word "model", I would go with "entire record object".

@jonathanhefner
Copy link
Member

Thank you, @AdityaBhutani!

rafaelfranca pushed a commit that referenced this pull request Jan 5, 2022
[ci skip] Pluck loads the records but omits attributes
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.

None yet

3 participants