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

Issue-138 Add Eager Load Option #139

Merged
merged 5 commits into from
Aug 30, 2023

Conversation

rafacoello
Copy link
Contributor

@rafacoello rafacoello commented Nov 26, 2021

  • Added eager load to default locators (in locate and locate_many methods) with backwards compatibility
  • Added tests cases with includes method in use.
  • Created a new stub class for mocking the includes behavior
    ~ Modified Person::Child class to have a relationship called parent.

@rafacoello rafacoello changed the title add eager load option Add eager load option Nov 26, 2021
@rafacoello rafacoello changed the title Add eager load option Add Eager Load Option Nov 26, 2021
@rafacoello rafacoello changed the title Add Eager Load Option Issue-138 Add Eager Load Option Dec 6, 2021
lib/global_id/locator.rb Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
lib/global_id/locator.rb Outdated Show resolved Hide resolved
lib/global_id/locator.rb Outdated Show resolved Hide resolved
lib/global_id/locator.rb Outdated Show resolved Hide resolved
lib/global_id/locator.rb Outdated Show resolved Hide resolved
lib/global_id/locator.rb Outdated Show resolved Hide resolved
lib/global_id/locator.rb Outdated Show resolved Hide resolved
@hosamaly
Copy link

This is a very valuable feature. Can it be released, please, @rafaelfranca?

@rafacoello
Copy link
Contributor Author

Hello @rafaelfranca . Should I do something else here or this is ok the way it is now? Thanks in advance

@rafaelfranca
Copy link
Member

Hey @rafacoello, it seems you forgot to deprecate the locator with only one argument, can you add that?

@rafacoello
Copy link
Contributor Author

Hey @rafacoello, it seems you forgot to deprecate the locator with only one argument, can you add that?

Hey @rafaelfranca Sorry. I miss-understood the first time. Please let me know if that way is Ok for the deprecation. Thanks!

return unless gid && find_allowed?(gid.model_class, options[:only])

if locator_for(gid).method(:locate).arity == 1
warn "#{Kernel.caller.first} warning: method locate(gid) is deprecated. Calling `locate(gid)' is deprecated. Please use `locate(gid, options)' instead."

Choose a reason for hiding this comment

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

Suggested change
warn "#{Kernel.caller.first} warning: method locate(gid) is deprecated. Calling `locate(gid)' is deprecated. Please use `locate(gid, options)' instead."
ActiveSupport::Deprecation.warn "Calling `locate(gid)' is deprecated. Please use `locate(gid, options)' instead."

Also see https://edgeguides.rubyonrails.org/contributing_to_ruby_on_rails.html#breaking-changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @bdewater Done. Thanks!

@rafacoello rafacoello requested review from bdewater and rafaelfranca and removed request for rafaelfranca and bdewater January 18, 2023 17:49
@rafaelfranca rafaelfranca force-pushed the add-eager-load-feature branch 2 times, most recently from fe4ab58 to bca0e65 Compare August 30, 2023 18:56
rafacoello and others added 5 commits August 30, 2023 18:56
+ added eager load to default locators (in locate and locate_many methods) with backwards compatibility
+ added tests cases with includes method in use.
+ created a new stub class for mocking the includes behavior
~ modified Person::Child class to have a relationship called parent.
@rafaelfranca rafaelfranca merged commit 1eff550 into rails:main Aug 30, 2023
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants