Permalink
Browse files

Deprecate InstanceMethods namespace handling in ActiveSupport::Concern.

This avoids the unnecessary "yo dawg, I heard you like include, so I put a module that includes your module when it is included" approach when building extensions.
  • Loading branch information...
josevalim committed Nov 21, 2011
1 parent f312e21 commit 401393b6561adc1ce7101945163c9601257c057a
Showing with 7 additions and 15 deletions.
  1. +5 −10 activesupport/lib/active_support/concern.rb
  2. +2 −5 activesupport/test/concern_test.rb
@@ -4,17 +4,12 @@ module ActiveSupport
# module M
# def self.included(base)
# base.extend ClassMethods
# base.send(:include, InstanceMethods)
# scope :disabled, where(:disabled => true)
# end
#
# module ClassMethods
# ...
# end
#
# module InstanceMethods
# ...
# end
# end
#
# By using <tt>ActiveSupport::Concern</tt> the above module could instead be written as:
@@ -31,10 +26,6 @@ module ActiveSupport
# module ClassMethods
# ...
# end
#
# module InstanceMethods
# ...
# end
# end
#
# Moreover, it gracefully handles module dependencies. Given a +Foo+ module and a +Bar+
@@ -118,7 +109,11 @@ def append_features(base)
@_dependencies.each { |dep| base.send(:include, dep) }
super
base.extend const_get("ClassMethods") if const_defined?("ClassMethods")
base.send :include, const_get("InstanceMethods") if const_defined?("InstanceMethods")
if const_defined?("InstanceMethods")
base.send :include, const_get("InstanceMethods")
ActiveSupport::Deprecation.warn "The InstanceMethods module inside ActiveSupport::Concern will be " \
"no longer included automatically. Please define instance methods directly in #{base} instead.", caller
end
base.class_eval(&@_included_block) if instance_variable_defined?("@_included_block")
end
end
@@ -19,9 +19,6 @@ def included_ran
end
end
module InstanceMethods
end
included do
self.included_ran = true
end
@@ -74,7 +71,7 @@ def test_class_methods_are_extended
def test_instance_methods_are_included
@klass.send(:include, Baz)
assert_equal "baz", @klass.new.baz
assert @klass.included_modules.include?(ConcernTest::Baz::InstanceMethods)
assert @klass.included_modules.include?(ConcernTest::Baz)
end
def test_included_block_is_ran
@@ -92,6 +89,6 @@ def test_modules_dependencies_are_met
def test_dependencies_with_multiple_modules
@klass.send(:include, Foo)
assert_equal [ConcernTest::Foo, ConcernTest::Bar, ConcernTest::Baz::InstanceMethods, ConcernTest::Baz], @klass.included_modules[0..3]
assert_equal [ConcernTest::Foo, ConcernTest::Bar, ConcernTest::Baz], @klass.included_modules[0..2]
end
end

8 comments on commit 401393b

@jlecour

This comment has been minimized.

Show comment
Hide comment
@jlecour

jlecour Nov 22, 2011

Contributor

Does this mean that we don't have to create an InstanceMethods module, and just put our methods in the parent module ?
It's a little less expressive, but definitely cleaner.

Contributor

jlecour replied Nov 22, 2011

Does this mean that we don't have to create an InstanceMethods module, and just put our methods in the parent module ?
It's a little less expressive, but definitely cleaner.

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Nov 22, 2011

Contributor
Contributor

josevalim replied Nov 22, 2011

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Nov 22, 2011

Member

👍

Member

fxn replied Nov 22, 2011

👍

@jodosha

This comment has been minimized.

Show comment
Hide comment
@jodosha

jodosha replied Nov 23, 2011

+1

@clemens

This comment has been minimized.

Show comment
Hide comment
@clemens

clemens Dec 7, 2011

Contributor

I hate you for this, Jose. ;-) In one of my projects I've started scoping everything to InstanceMethods because I thought there was a good reason that Rails was doing it. Now I'll have to fix something that was right in the first place. -_- Luckily, it's just one git revert away. :D

Contributor

clemens replied Dec 7, 2011

I hate you for this, Jose. ;-) In one of my projects I've started scoping everything to InstanceMethods because I thought there was a good reason that Rails was doing it. Now I'll have to fix something that was right in the first place. -_- Luckily, it's just one git revert away. :D

@phillipoertel

This comment has been minimized.

Show comment
Hide comment
@phillipoertel

phillipoertel Feb 13, 2012

In the following example:

class Project < ActiveRecord::Base
include Project::Visibility
...

module Project::Visibility
extend ActiveSupport::Concern
...
module InstanceMethods
...

I get the deprecation warning: "The InstanceMethods module inside ActiveSupport::Concern will be no longer included automatically. Please define instance methods directly in Project instead. (called from include at /Users/phillip/Projekte/torial/git/app/models/project.rb:3)"

IMHO the deprecation warning should tell me to put the instance methods directly into the module, not the base class? Putting the code back into the main class kind of spoils the whole concern idea, right?

The implementation is correct, it's just the deprecation warning that is confusing.

phillipoertel replied Feb 13, 2012

In the following example:

class Project < ActiveRecord::Base
include Project::Visibility
...

module Project::Visibility
extend ActiveSupport::Concern
...
module InstanceMethods
...

I get the deprecation warning: "The InstanceMethods module inside ActiveSupport::Concern will be no longer included automatically. Please define instance methods directly in Project instead. (called from include at /Users/phillip/Projekte/torial/git/app/models/project.rb:3)"

IMHO the deprecation warning should tell me to put the instance methods directly into the module, not the base class? Putting the code back into the main class kind of spoils the whole concern idea, right?

The implementation is correct, it's just the deprecation warning that is confusing.

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Feb 13, 2012

Contributor

Yeah, the deprecation warning is wrong, fixing.

Contributor

josevalim replied Feb 13, 2012

Yeah, the deprecation warning is wrong, fixing.

@phillipoertel

This comment has been minimized.

Show comment
Hide comment
@phillipoertel

phillipoertel Feb 13, 2012

you are awesome! :-)

phillipoertel replied Feb 13, 2012

you are awesome! :-)

Please sign in to comment.