Skip to content

Use find_by_sql instead of select_all and send(:instantiate) #57

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Use find_by_sql instead of select_all and send(:instantiate) #57

wants to merge 2 commits into from

Conversation

jhawthorn
Copy link
Contributor

These are both private APIs, but _load_from_sql will give us even more standard behaviour (this is what find_by_sql calls) including emitting the instantiation.active_record notification.

This may be faster in some cases since _load_from_sql includes a fast path for when an inheritance column isn't present.

zerowidth
zerowidth approved these changes Sep 23, 2021
View reviewed changes
Copy link
Member

@zerowidth zerowidth left a comment

Choose a reason for hiding this comment

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

You'd know better than me!

@jhawthorn
Use _load_from_sql instead of send(:instantiate)
597abcd 
These are both private APIs, but _load_from_sql will give us even more
standard behaviour (this is what find_by_sql calls) including emitting
the instantiation.active_record notification.

This may be faster in some cases since _load_from_sql includes a fast
path for when an inheritance column isn't present.
@jhawthorn jhawthorn changed the title Use _load_from_sql instead of send(:instantiate) Use find_by_sql instead of send(:instantiate) Sep 23, 2021
@jhawthorn
Copy link
Contributor Author

Updated to just call find_by_sql. This is a slight behaviour change in that we'll now print out ex. "User Load" instead of "User Load via GitHub::SQL".

This move us to using a completely standard API 🎉

@jhawthorn jhawthorn changed the title Use find_by_sql instead of send(:instantiate) Use find_by_sql instead of select_all and send(:instantiate) Sep 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zerowidth zerowidth zerowidth approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jhawthorn @zerowidth