From 1eee0ca6de975b42524105a59e0521d18b38ab81 Mon Sep 17 00:00:00 2001 From: Jeremy Kemper Date: Tue, 17 Dec 2013 02:31:57 -0700 Subject: [PATCH] Introduce Module#concerning A natural, low-ceremony way to separate responsibilities within a class. Imported from https://github.com/37signals/concerning#readme --- activesupport/CHANGELOG.md | 47 ++++++ .../lib/active_support/core_ext/module.rb | 1 + .../core_ext/module/concerning.rb | 136 ++++++++++++++++++ .../test/core_ext/module/concerning_test.rb | 35 +++++ 4 files changed, 219 insertions(+) create mode 100644 activesupport/lib/active_support/core_ext/module/concerning.rb create mode 100644 activesupport/test/core_ext/module/concerning_test.rb diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md index 994e44db00..1945572bf7 100644 --- a/activesupport/CHANGELOG.md +++ b/activesupport/CHANGELOG.md @@ -1,3 +1,50 @@ +* Introduce Module#concerning: a natural, low-ceremony way to separate + responsibilities within a class. + + Imported from https://github.com/37signals/concerning#readme + + class Todo < ActiveRecord::Base + concerning :EventTracking do + included do + has_many :events + end + + def latest_event + ... + end + + private + def some_internal_method + ... + end + end + + concerning :Trashable do + def trashed? + ... + end + + def latest_event + super some_option: true + end + end + end + + is equivalent to defining these modules inline, extending them into + concerns, then mixing them in to the class. + + Inline concerns tame "junk drawer" classes that intersperse many unrelated + class-level declarations, public instance methods, and private + implementation. Coalesce related bits and give them definition. + These are a stepping stone toward future growth & refactoring. + + When to move on from an inline concern: + * Encapsulating state? Extract collaborator object. + * Encompassing more public behavior or implementation? Move to separate file. + * Sharing behavior among classes? Move to separate file. + + *Jeremy Kemper* + * Fix file descriptor being leaked on each call to `Kernel.silence_stream`. *Mario Visic* diff --git a/activesupport/lib/active_support/core_ext/module.rb b/activesupport/lib/active_support/core_ext/module.rb index f2d4887df6..b4efff8b24 100644 --- a/activesupport/lib/active_support/core_ext/module.rb +++ b/activesupport/lib/active_support/core_ext/module.rb @@ -4,6 +4,7 @@ require 'active_support/core_ext/module/anonymous' require 'active_support/core_ext/module/reachable' require 'active_support/core_ext/module/attribute_accessors' require 'active_support/core_ext/module/attr_internal' +require 'active_support/core_ext/module/concerning' require 'active_support/core_ext/module/delegation' require 'active_support/core_ext/module/deprecation' require 'active_support/core_ext/module/remove_method' diff --git a/activesupport/lib/active_support/core_ext/module/concerning.rb b/activesupport/lib/active_support/core_ext/module/concerning.rb new file mode 100644 index 0000000000..1e151e0ee7 --- /dev/null +++ b/activesupport/lib/active_support/core_ext/module/concerning.rb @@ -0,0 +1,136 @@ +require 'active_support/concern' + +class Module + # = Bite-sized separation of concerns + # + # We often find ourselves with a medium-sized chunk of behavior that we'd + # like to extract, but only mix in to a single class. + # + # Extracting a plain old Ruby object to encapsulate it and collaborate or + # delegate to the original object is often a good choice, but when there's + # no additional state to encapsulate or we're making DSL-style declarations + # about the parent class, introducing new collaborators can obfuscate rather + # than simplify. + # + # The typical route is to just dump everything in a monolithic class, perhaps + # with a comment, as a least-bad alternative. Using modules in separate files + # means tedious sifting to get a big-picture view. + # + # = Dissatisfying ways to separate small concerns + # + # == Using comments: + # + # class Todo + # # Other todo implementation + # # ... + # + # ## Event tracking + # has_many :events + # + # before_create :track_creation + # after_destroy :track_deletion + # + # private + # def track_creation + # # ... + # end + # end + # + # == With an inline module: + # + # Noisy syntax. + # + # class Todo + # # Other todo implementation + # # ... + # + # module EventTracking + # extend ActiveSupport::Concern + # + # included do + # has_many :events + # before_create :track_creation + # after_destroy :track_deletion + # end + # + # private + # def track_creation + # # ... + # end + # end + # include EventTracking + # end + # + # == Mix-in noise exiled to its own file: + # + # Once our chunk of behavior starts pushing the scroll-to-understand it + # boundary, we give in and move it to a separate file. At this size, the + # overhead feels in good proportion to the size of our extraction, despite + # diluting our at-a-glance sense of how things really work. + # + # class Todo + # # Other todo implementation + # # ... + # + # include TodoEventTracking + # end + # + # = Introducing Module#concerning + # + # By quieting the mix-in noise, we arrive at a natural, low-ceremony way to + # separate bite-sized concerns. + # + # class Todo + # # Other todo implementation + # # ... + # + # concerning :EventTracking do + # included do + # has_many :events + # before_create :track_creation + # after_destroy :track_deletion + # end + # + # private + # def track_creation + # # ... + # end + # end + # end + # + # Todo.ancestors + # # => Todo, Todo::EventTracking, Object + # + # This small step has some wonderful ripple effects. We can + # * grok the behavior of our class in one glance, + # * clean up monolithic junk-drawer classes by separating their concerns, and + # * stop leaning on protected/private for crude "this is internal stuff" modularity. + module Concerning + # Define a new concern and mix it in. + def concerning(topic, &block) + include concern(topic, &block) + end + + # A low-cruft shortcut to define a concern. + # + # concern :EventTracking do + # ... + # end + # + # is equivalent to + # + # module EventTracking + # extend ActiveSupport::Concern + # + # ... + # end + # include EventTracking + def concern(topic, &module_definition) + const_set topic, Module.new { + extend ::ActiveSupport::Concern + module_eval(&module_definition) + } + end + end + include Concerning +end diff --git a/activesupport/test/core_ext/module/concerning_test.rb b/activesupport/test/core_ext/module/concerning_test.rb new file mode 100644 index 0000000000..c6863b24a4 --- /dev/null +++ b/activesupport/test/core_ext/module/concerning_test.rb @@ -0,0 +1,35 @@ +require 'abstract_unit' +require 'active_support/core_ext/module/concerning' + +class ConcerningTest < ActiveSupport::TestCase + def test_concern_shortcut_creates_a_module_but_doesnt_include_it + mod = Module.new { concern(:Foo) { } } + assert_kind_of Module, mod::Foo + assert mod::Foo.respond_to?(:included) + assert !mod.ancestors.include?(mod::Foo), mod.ancestors.inspect + end + + def test_concern_creates_a_module_extended_with_active_support_concern + klass = Class.new do + concern :Foo do + included { @foo = 1 } + def should_be_public; end + end + end + + # Declares a concern but doesn't include it + assert_kind_of Module, klass::Foo + assert !klass.ancestors.include?(klass::Foo), klass.ancestors.inspect + + # Public method visibility by default + assert klass::Foo.public_instance_methods.map(&:to_s).include?('should_be_public') + + # Calls included hook + assert_equal 1, Class.new { include klass::Foo }.instance_variable_get('@foo') + end + + def test_concerning_declares_a_concern_and_includes_it_immediately + klass = Class.new { concerning(:Foo) { } } + assert klass.ancestors.include?(klass::Foo), klass.ancestors.inspect + end +end