Skip to content

Conversation

@sobakasu
Copy link
Contributor

@sobakasu sobakasu commented Jan 27, 2026

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:

class ApplicationCollection < Katalyst::Tables::Collection::Base
  config.paginate = {}
end

class SomeController
  class Collection < ApplicationCollection
    # controller specific settings
  end
end

The expectation being that config in ApplicationCollection is inherited by SomeController::Collection. That doesn't seem to happen, as we use a class variable @config which is not inherited. This PR changes @config to a class_attribute, and dups it for subclasses. And added some tests.

@sobakasu sobakasu requested a review from sfnelson January 27, 2026 01:47
Copy link
Contributor

@sfnelson sfnelson left a 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?

@sobakasu sobakasu force-pushed the feature/core-config-class-attribute branch 2 times, most recently from 78f4f4a to 6a27476 Compare January 27, 2026 23:01
@sobakasu sobakasu force-pushed the feature/core-config-class-attribute branch from 6a27476 to 3524866 Compare January 30, 2026 02:52
@sobakasu sobakasu changed the title Support for config inheritance Re-implement config inheritance support after removal of ActiveSupport::Configurable Jan 30, 2026
@sfnelson sfnelson merged commit d0d6fc9 into main Jan 30, 2026
1 check passed
@sfnelson sfnelson deleted the feature/core-config-class-attribute branch January 30, 2026 04:20
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.

3 participants