Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove explicit respond_to? and Validator#setup call #10716

10 changes: 5 additions & 5 deletions activemodel/lib/active_model/validations/acceptance.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,11 @@ def validate_each(record, attribute, value)
end
end

def setup(klass)
attr_readers = attributes.reject { |name| klass.attribute_method?(name) }
attr_writers = attributes.reject { |name| klass.attribute_method?("#{name}=") }
klass.send(:attr_reader, *attr_readers)
klass.send(:attr_writer, *attr_writers)
def setup!
attr_readers = attributes.reject { |name| @klass.attribute_method?(name) }
attr_writers = attributes.reject { |name| @klass.attribute_method?("#{name}=") }
@klass.send(:attr_reader, *attr_readers)
@klass.send(:attr_writer, *attr_writers)
end
end

Expand Down
10 changes: 5 additions & 5 deletions activemodel/lib/active_model/validations/confirmation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,13 @@ def validate_each(record, attribute, value)
end
end

def setup(klass)
klass.send(:attr_reader, *attributes.map do |attribute|
:"#{attribute}_confirmation" unless klass.method_defined?(:"#{attribute}_confirmation")
def setup!
Copy link
Member

Choose a reason for hiding this comment

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

Should not this method be private now?

@klass.send(:attr_reader, *attributes.map do |attribute|
:"#{attribute}_confirmation" unless @klass.method_defined?(:"#{attribute}_confirmation")
end.compact)

klass.send(:attr_writer, *attributes.map do |attribute|
:"#{attribute}_confirmation" unless klass.method_defined?(:"#{attribute}_confirmation=")
@klass.send(:attr_writer, *attributes.map do |attribute|
:"#{attribute}_confirmation" unless @klass.method_defined?(:"#{attribute}_confirmation=")
end.compact)
end
end
Expand Down
3 changes: 2 additions & 1 deletion activemodel/lib/active_model/validations/with.rb
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,10 @@ module ClassMethods
# end
def validates_with(*args, &block)
options = args.extract_options!
options[:class] = self

args.each do |klass|
validator = klass.new(options, &block)
validator.setup(self) if validator.respond_to?(:setup)

if validator.respond_to?(:attributes) && !validator.attributes.empty?
validator.attributes.each do |attribute|
Expand Down
39 changes: 30 additions & 9 deletions activemodel/lib/active_model/validator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -82,18 +82,16 @@ module ActiveModel
# validates :title, presence: true
# end
#
# Validator may also define a +setup+ instance method which will get called
# with the class that using that validator as its argument. This can be
# useful when there are prerequisites such as an +attr_accessor+ being present.
# Validator may also define a +setup!+ instance method which will get called
# automatically in the constructor. It can be useful to access the class that is
# using that validator when there are prerequisites such as an +attr_accessor+ being present.
# This class is accessable via +@klass+.
#
# class MyValidator < ActiveModel::Validator
# def setup(klass)
# klass.send :attr_accessor, :custom_attribute
# def setup!
# @klass.send :attr_accessor, :custom_attribute
# end
# end
#
# This setup method is only called when used with validation macros or the
# class level <tt>validates_with</tt> method.
class Validator
attr_reader :options

Expand All @@ -107,7 +105,9 @@ def self.kind

# Accepts options that will be made available through the +options+ reader.
def initialize(options = {})
@options = options.freeze
@klass = options[:class]
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the need for always storing the @klass if we only need it in some validators and temporarily.

@options = options.except(:class).freeze
deprecated_setup or setup!
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if we stil have Rails guidelines around but part of them was about never using and or or due to precedence rules.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, can't we just let developers override initialize? Why still call it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had this in an earlier version, for me it felt ok but I wasn't sure if you guys like the additional call. So, override initialize in validators:

def initialize(options)
  super
  @klass = options[:class]

end

# Return the kind for this validator.
Expand All @@ -123,6 +123,27 @@ def kind
def validate(record)
raise NotImplementedError, "Subclasses must implement a validate(record) method."
end

private
# Override this private method to setup the validator.
def setup!
Copy link
Member

Choose a reason for hiding this comment

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

This should be indented one level mode than the private keyword

Copy link
Member

Choose a reason for hiding this comment

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

I think we should change the documentation of this method on the Validator class

end

def deprecated_setup # TODO: remove me in 4.2.
return unless respond_to?(:setup)
ActiveSupport::Deprecation.warn "The `Validator#setup` instance method is deprecated and will be removed on Rails 4.2. Change your `setup` method to something like:

Copy link
Member

Choose a reason for hiding this comment

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

@apotonick should not the deprecation warning be updated to show the initialize override example?

class MyValidator < ActiveModel::Validator
private

def setup!
# you have access to @klass here:
prepare_something_with(@klass)
end
end
"
setup(@klass)
end
end

# +EachValidator+ is a validator which iterates through the attributes given
Expand Down
24 changes: 1 addition & 23 deletions activemodel/test/cases/validations/with_validation_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -100,35 +100,13 @@ def check_validity!
test "passes all configuration options to the validator class" do
topic = Topic.new
validator = mock()
validator.expects(:new).with(foo: :bar, if: "1 == 1").returns(validator)
validator.expects(:new).with(foo: :bar, if: "1 == 1", class: Topic).returns(validator)
validator.expects(:validate).with(topic)

Topic.validates_with(validator, if: "1 == 1", foo: :bar)
assert topic.valid?
end

test "calls setup method of validator passing in self when validator has setup method" do
topic = Topic.new
validator = stub_everything
validator.stubs(:new).returns(validator)
validator.stubs(:validate)
validator.stubs(:respond_to?).with(:setup).returns(true)
validator.expects(:setup).with(Topic).once
Topic.validates_with(validator)
assert topic.valid?
end

test "doesn't call setup method of validator when validator has no setup method" do
topic = Topic.new
validator = stub_everything
validator.stubs(:new).returns(validator)
validator.stubs(:validate)
validator.stubs(:respond_to?).with(:setup).returns(false)
validator.expects(:setup).with(Topic).never
Topic.validates_with(validator)
assert topic.valid?
end

test "validates_with with options" do
Topic.validates_with(ValidatorThatValidatesOptions, field: :first_name)
topic = Topic.new
Expand Down
21 changes: 21 additions & 0 deletions activemodel/test/cases/validations_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -373,4 +373,25 @@ def test_dup_validity_is_independent
assert topic.invalid?
assert duped.valid?
end

# validator test:
def test_setup_is_deprecated_but_still_receives_klass # TODO: remove me in 4.2.
validator_class = Class.new(ActiveModel::Validator) do
def setup(klass)
@old_klass = klass
end

def validate(*)
@old_klass == Topic or raise "#setup didn't work"
end
end

assert_deprecated do
Topic.validates_with validator_class
end

t = Topic.new
t.valid?
end

end
5 changes: 0 additions & 5 deletions activerecord/lib/active_record/validations/uniqueness.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,6 @@ def initialize(options)
super({ case_sensitive: true }.merge!(options))
end

# Unfortunately, we have to tie Uniqueness validators to a class.
def setup(klass)
@klass = klass
end

def validate_each(record, attribute, value)
finder_class = find_finder_class_for(record)
table = finder_class.arel_table
Expand Down