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

Support setting parent_controller #903

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

Conversation

jaynetics
Copy link

fixes #618

for the interface, i mirrored Devise::parent_controller=.

Copy link

codecov bot commented Mar 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.92%. Comparing base (cafbfbf) to head (a156fb0).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #903   +/-   ##
=======================================
  Coverage   98.91%   98.92%           
=======================================
  Files          14       14           
  Lines         555      556    +1     
=======================================
+ Hits          549      550    +1     
  Misses          6        6           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jaynetics jaynetics force-pushed the support-setting-parent-controller branch from 405c7c7 to a156fb0 Compare July 14, 2024 09:16
@jaynetics
Copy link
Author

rebased to bring up-to-date with master.

@tagliala could we get this one merged?

force_parent_controller('ActionController::Base')

assert_equal ActionController::Base, InheritedResources::Base.superclass
force_parent_controller(original_parent.to_s) # restore original parent
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be in something like ensure ?

Copy link
Author

Choose a reason for hiding this comment

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

@tagliala done.

@jaynetics jaynetics force-pushed the support-setting-parent-controller branch from a156fb0 to 78d7b34 Compare July 15, 2024 08:39
@jaynetics jaynetics force-pushed the support-setting-parent-controller branch from 78d7b34 to 817d23d Compare July 15, 2024 08:41

# Inherit from a different controller. This only has an effect if changed
# before InheritedResources::Base is loaded, e.g. in a rails initializer.
mattr_accessor(:parent_controller) { '::ApplicationController' }
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

@tagliala i'd say its equivalent – the block is evaluated and the class variable is set immediately:

https://github.com/rails/rails/blob/56fbc632a378acc40d99f5b4be321f80a5d8b8c4/activesupport/lib/active_support/core_ext/module/attribute_accessors.rb#L69-L70

i think the devise code is just like that because when José Valim wrote these lines 13 years ago, mattr_accessor did not yet support passing a default value:

https://github.com/rails/rails/blob/bf526c2dbeb73bf11553004e43889a804b72866d/activesupport/lib/active_support/core_ext/module/attribute_accessors.rb#L60

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.

Add an option to provide another superclass other than ApplicationController
3 participants