Skip to content

Commit

Permalink
Introduce Concern#class_methods and Kernel#concern
Browse files Browse the repository at this point in the history
  • Loading branch information
jeremy committed Feb 23, 2014
1 parent 96759cf commit b16c36e
Show file tree
Hide file tree
Showing 7 changed files with 113 additions and 18 deletions.
28 changes: 28 additions & 0 deletions activesupport/CHANGELOG.md
@@ -1,3 +1,31 @@
* Introduce `Concern#class_methods` as a sleek alternative to clunky
`module ClassMethods`. Add `Kernel#concern` to define at the toplevel
without chunky `module Foo; extend ActiveSupport::Concern` boilerplate.

# app/models/concerns/authentication.rb
concern :Authentication do
included do
after_create :generate_private_key
end

class_methods do
def authenticate(credentials)
# ...
end
end

def generate_private_key
# ...
end
end

# app/models/user.rb
class User < ActiveRecord::Base
include Authentication
end

*Jeremy Kemper*

* Added `Object#present_in` to simplify value whitelisting.

Before:
Expand Down
10 changes: 9 additions & 1 deletion activesupport/lib/active_support/concern.rb
Expand Up @@ -26,7 +26,7 @@ module ActiveSupport
# scope :disabled, -> { where(disabled: true) }
# end
#
# module ClassMethods
# class_methods do
# ...
# end
# end
Expand Down Expand Up @@ -130,5 +130,13 @@ def included(base = nil, &block)
super
end
end

def class_methods(&class_methods_module_definition)
mod = const_defined?(:ClassMethods) ?
const_get(:ClassMethods) :
const_set(:ClassMethods, Module.new)

mod.module_eval(&class_methods_module_definition)
end
end
end
3 changes: 2 additions & 1 deletion activesupport/lib/active_support/core_ext/kernel.rb
@@ -1,4 +1,5 @@
require 'active_support/core_ext/kernel/reporting'
require 'active_support/core_ext/kernel/agnostics'
require 'active_support/core_ext/kernel/concern'
require 'active_support/core_ext/kernel/debugger'
require 'active_support/core_ext/kernel/reporting'
require 'active_support/core_ext/kernel/singleton_class'
10 changes: 10 additions & 0 deletions activesupport/lib/active_support/core_ext/kernel/concern.rb
@@ -0,0 +1,10 @@
require 'active_support/core_ext/module/concerning'

module Kernel
# A shortcut to define a toplevel concern, not within a module.
#
# See ActiveSupport::CoreExt::Module::Concerning for more.
def concern(topic, &module_definition)
Object.concern topic, &module_definition
end
end
10 changes: 8 additions & 2 deletions activesupport/test/concern_test.rb
Expand Up @@ -5,7 +5,7 @@ class ConcernTest < ActiveSupport::TestCase
module Baz
extend ActiveSupport::Concern

module ClassMethods
class_methods do
def baz
"baz"
end
Expand Down Expand Up @@ -33,6 +33,12 @@ module Bar

include Baz

module ClassMethods
def baz
"bar's baz + " + super
end
end

def bar
"bar"
end
Expand Down Expand Up @@ -73,7 +79,7 @@ def test_modules_dependencies_are_met
@klass.send(:include, Bar)
assert_equal "bar", @klass.new.bar
assert_equal "bar+baz", @klass.new.baz
assert_equal "baz", @klass.baz
assert_equal "bar's baz + baz", @klass.baz
assert @klass.included_modules.include?(ConcernTest::Bar)
end

Expand Down
12 changes: 12 additions & 0 deletions activesupport/test/core_ext/kernel/concern_test.rb
@@ -0,0 +1,12 @@
require 'abstract_unit'
require 'active_support/core_ext/kernel/concern'

class KernelConcernTest < ActiveSupport::TestCase
def test_may_be_defined_at_toplevel
mod = ::TOPLEVEL_BINDING.eval 'concern(:ToplevelConcern) { }'
assert_equal mod, ::ToplevelConcern
assert_kind_of ActiveSupport::Concern, ::ToplevelConcern
assert !Object.ancestors.include?(::ToplevelConcern), mod.ancestors.inspect
Object.send :remove_const, :ToplevelConcern
end
end
58 changes: 44 additions & 14 deletions activesupport/test/core_ext/module/concerning_test.rb
@@ -1,35 +1,65 @@
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
class ModuleConcerningTest < ActiveSupport::TestCase
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

class ModuleConcernTest < ActiveSupport::TestCase
def test_concern_creates_a_module_extended_with_active_support_concern
klass = Class.new do
concern :Foo do
concern :Baz 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
assert klass.const_defined?(:Baz, false)
assert !ModuleConcernTest.const_defined?(:Baz)
assert_kind_of ActiveSupport::Concern, klass::Baz
assert !klass.ancestors.include?(klass::Baz), klass.ancestors.inspect

# Public method visibility by default
assert klass::Foo.public_instance_methods.map(&:to_s).include?('should_be_public')
assert klass::Baz.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')
assert_equal 1, Class.new { include klass::Baz }.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
class Foo
concerning :Bar do
module ClassMethods
def will_be_orphaned; end
end

const_set :ClassMethods, Module.new {
def hacked_on; end
}

# Doesn't overwrite existing ClassMethods module.
class_methods do
def nicer_dsl; end
end

# Doesn't overwrite previous class_methods definitions.
class_methods do
def doesnt_clobber; end
end
end
end

def test_using_class_methods_blocks_instead_of_ClassMethods_module
assert !Foo.respond_to?(:will_be_orphaned)
assert Foo.respond_to?(:hacked_on)
assert Foo.respond_to?(:nicer_dsl)
assert Foo.respond_to?(:doesnt_clobber)

# Orphan in Foo::ClassMethods, not Bar::ClassMethods.
assert Foo.const_defined?(:ClassMethods)
assert Foo::ClassMethods.method_defined?(:will_be_orphaned)
end
end

5 comments on commit b16c36e

@et
Copy link

@et et commented on b16c36e Feb 23, 2014

Choose a reason for hiding this comment

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

Was it really that clunky? This seems to be reintroducing "magic" back to rails.
Not to complain, just wondering if there was a sound miscomprehension of module Foo; extend ActiveSupport::Concern

@sobrinho
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm really against this, there is no clunky with the current way.

Changing this:

# app/models/concerns/authentication.rb
module Authentication
  extend ActiveSupport::Concern

  included do
    after_create :generate_private_key
  end

  module ClassMethods
    def authenticate(credentials)
      # ...
    end
  end

  def generate_private_key
    # ...
  end
end

# app/models/user.rb
class User < ActiveRecord::Base
  include Authentication
end

To this:

# app/models/concerns/authentication.rb
concern :Authentication do
  included do
    after_create :generate_private_key
  end

  class_methods do
    def authenticate(credentials)
      # ...
    end
  end

  def generate_private_key
    # ...
  end
end

# app/models/user.rb
class User < ActiveRecord::Base
  include Authentication
end

Creates magic without any benefit.

@arthurnn
Copy link
Member

Choose a reason for hiding this comment

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

@sobrinho you still can do the old style.

@sobrinho
Copy link
Contributor

Choose a reason for hiding this comment

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

@arthurnn sure, but from the time that something like this enters on core it becomes a standard.

@fxn
Copy link
Member

@fxn fxn commented on b16c36e May 25, 2014

Choose a reason for hiding this comment

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

included and module ClassMethods are "magic" disguised as ordinary Ruby. These macros make clear you are doing something special. In addition, in my view, the code looks much simpler and the intention is clear.

PS: Rails has no "magic", if you see a concern macro you go to the documentation and see what it deterministically does.

Please sign in to comment.