Skip to content

Commit

Permalink
Remove special case filtering for Procs.
Browse files Browse the repository at this point in the history
I'm writing this patch for two purposes:

1. I want to reduce the number of times `object_id` is called.  Calling
   `object_id` can have negative impacts on performance in Ruby 2.7+, so
   it would be nice to stop calling it.

2. I'm not sure why we're treating lambdas specially here.  It looks
   like we wanted to prevent people from skipping callbacks that were
   defined with a lambda, but I think that is silly.  If the user has a
   reference to a lambda, and they want to skip it, we should let them.

I think this cleans up some code, helps with performance, and is a more
intuitive interface.
  • Loading branch information
tenderlove committed Mar 3, 2021
1 parent 7b9cfde commit d5ac941
Show file tree
Hide file tree
Showing 4 changed files with 6 additions and 28 deletions.
2 changes: 1 addition & 1 deletion actionpack/test/controller/filters_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ class << self

def before_actions
filters = _process_action_callbacks.select { |c| c.kind == :before }
filters.map!(&:raw_filter)
filters.map!(&:filter)
end
end
end
Expand Down
15 changes: 1 addition & 14 deletions activesupport/lib/active_support/callbacks.rb
Original file line number Diff line number Diff line change
Expand Up @@ -289,21 +289,17 @@ def self.build(chain, filter, kind, options)
end

attr_accessor :kind, :name
attr_reader :chain_config
attr_reader :chain_config, :filter

def initialize(name, filter, kind, options, chain_config)
@chain_config = chain_config
@name = name
@kind = kind
@filter = filter
@key = compute_identifier filter
@if = check_conditionals(options[:if])
@unless = check_conditionals(options[:unless])
end

def filter; @key; end
def raw_filter; @filter; end

def merge_conditional_options(chain, if_option:, unless_option:)
options = {
if: @if.dup,
Expand Down Expand Up @@ -367,15 +363,6 @@ def check_conditionals(conditionals)
conditionals.freeze
end

def compute_identifier(filter)
case filter
when ::Proc
filter.object_id
else
filter
end
end

def conditions_lambdas
@if.map { |c| CallTemplate.build(c, self).make_lambda } +
@unless.map { |c| CallTemplate.build(c, self).inverted_lambda }
Expand Down
9 changes: 0 additions & 9 deletions activesupport/test/callbacks_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1129,15 +1129,6 @@ def test_skip_class # removes one at a time
}
end

def test_skip_lambda # raises error
calls = []
callback = ->(o) { calls << o }
klass = build_class(callback)
assert_raises(ArgumentError) { klass.skip callback }
klass.new.run
assert_equal 10, calls.length
end

def test_skip_symbol # removes all
calls = []
klass = build_class(:bar)
Expand Down
8 changes: 4 additions & 4 deletions activesupport/test/test_case_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -325,9 +325,9 @@ class SetupAndTeardownTest < ActiveSupport::TestCase
teardown :foo, :sentinel

def test_inherited_setup_callbacks
assert_equal [:reset_callback_record, :foo], self.class._setup_callbacks.map(&:raw_filter)
assert_equal [:reset_callback_record, :foo], self.class._setup_callbacks.map(&:filter)
assert_equal [:foo], @called_back
assert_equal [:foo, :sentinel], self.class._teardown_callbacks.map(&:raw_filter)
assert_equal [:foo, :sentinel], self.class._teardown_callbacks.map(&:filter)
end

def setup
Expand Down Expand Up @@ -355,9 +355,9 @@ class SubclassSetupAndTeardownTest < SetupAndTeardownTest
teardown :bar

def test_inherited_setup_callbacks
assert_equal [:reset_callback_record, :foo, :bar], self.class._setup_callbacks.map(&:raw_filter)
assert_equal [:reset_callback_record, :foo, :bar], self.class._setup_callbacks.map(&:filter)
assert_equal [:foo, :bar], @called_back
assert_equal [:foo, :sentinel, :bar], self.class._teardown_callbacks.map(&:raw_filter)
assert_equal [:foo, :sentinel, :bar], self.class._teardown_callbacks.map(&:filter)
end

private
Expand Down

0 comments on commit d5ac941

Please sign in to comment.