-
Notifications
You must be signed in to change notification settings - Fork 0
Support for config inheritance #137
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
base: main
Are you sure you want to change the base?
Conversation
sfnelson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change makes sense but it's a response to a wider deprecation of ActiveSupport::Configurable in Rails 8.1. I took a 'light' approach in this plus the other gems like content and koi, rather than the much more complex approach Rails took internally. Is this approach consistent with how Rails has tackled the deprecation?
katalyst-tables.gemspec
Outdated
|
|
||
| spec.require_paths = ["lib"] | ||
|
|
||
| spec.add_dependency "activesupport" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also require activemodel. I think it's ok to assume Rails for this project and not add it to the gem spec.
| @config ||= Config.new | ||
| def inherited(subclass) | ||
| super | ||
| subclass.config = config.dup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rails takes a similar but different approach that might be clearer: rails/rails@1719d25
class_attribute :config, instance_predicate: false, default: ActiveSupport::OrderedOptions.new
class_methods do
def inherited(klass)
klass.config = ActiveSupport::InheritableOptions.new(config)
super
end
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ActiveSupport::InheritableOptions looks like it's designed to work with hashes, or OrderedOptions (which extends Hash). We are using Base::Config class for config. We could stick with .dup, or update config to use OrderedOptions (but then we'd be accepting any attribute rather than just paginate and sorting)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant the new approach, vs dup, and calling it before super
3905c9b to
78f4f4a
Compare
78f4f4a to
6a27476
Compare
Some of our projects (glg, gus, nfsa-pro) use Collections::Base with inheritance. They define an 'ApplicationCollection' with common settings and then inherit from that. Example code:
The expectation being that config in ApplicationCollection is inherited by SomeController::Collection. That doesn't seem to happen, as we use a class variable
@configwhich is not inherited. This PR changes@configto aclass_attribute, anddups it for subclasses. And added some tests.