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

Conversation

@apotonick
Copy link
Contributor

apotonick commented May 22, 2013

This moves the setup process of validators into the constructor, pushing all knowledge about this into the validator class itself. Makes it easier to override internals in subclassed validators.

…. instead, respective validators take care of their setup.
@rafaelfranca
Copy link
Member

rafaelfranca commented May 22, 2013

Could you give us example of what you want to accomplish with this change?

Maybe an example how was the validator subclass before and after it.

@apotonick
Copy link
Contributor Author

apotonick commented May 22, 2013

This is the "bottom line" of my PR: apotonick@543c59d#L2L88

I could get my stuff working using the existing code. However, my main intend is to remove the respond_to? call and move stuff into Validator itself. While this simplifies the Validator interface it is also preparing some more changes I'd like to push (especially simplifying UniquenessValidator in AR).

Are you worried about the change? Is it because the options contain a :class now (I don't like it either)? It shouldn't break existing code?!

@@ -123,6 +125,10 @@ def kind
def validate(record)
raise NotImplementedError, "Subclasses must implement a validate(record) method."
end

private

This comment has been minimized.

@rafaelfranca

rafaelfranca May 22, 2013 Member

This should be in the same indentation of the def.

@@ -123,6 +125,10 @@ def kind
def validate(record)
raise NotImplementedError, "Subclasses must implement a validate(record) method."
end

private
def setup!

This comment has been minimized.

@rafaelfranca

rafaelfranca May 22, 2013 Member

This should be indented one level mode than the private keyword

This comment has been minimized.

@rafaelfranca

rafaelfranca May 22, 2013 Member

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

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

This comment has been minimized.

@rafaelfranca

rafaelfranca May 22, 2013 Member

Should not this method be private now?

@rafaelfranca
Copy link
Member

rafaelfranca commented May 22, 2013

Are you worried about the change?

No. I just want to know more about the motivation of this pull request.

Is it because the options contain a :class now (I don't like it either)?

It is awkward, but it is fine to me.

It shouldn't break existing code?!

I'm not sure of this. setup(klass) is a public method and documented as part of the API (see Validator documentation). We can't just remove it.

@apotonick
Copy link
Contributor Author

apotonick commented May 22, 2013

You're right! #setup! should be made private soon and called by a public #setup which will be deprecated.

def setup(*)
  deprecation_warning
  setup!
end

private
def setup!
end
@rafaelfranca
Copy link
Member

rafaelfranca commented May 22, 2013

👍

@apotonick
Copy link
Contributor Author

apotonick commented May 22, 2013

What is the prefered way of deprecating (and testing this)? Sorry to ask :-)

@rafaelfranca
Copy link
Member

rafaelfranca commented May 22, 2013

Maybe:

if validator.respond_to?(:setup)
  ActiveSupport::Deprecation.warn 'The `setup` instance method is deprecated and will be removed on Rails 4.2. Define `setup!` without arguments instead.'
  validator.setup(self)
end

You can even provide a code snippet in the deprecation message to make explicit what the users have to do:

Change your setup method to:

class MyValidator < ActiveModel::Validator
  private

  def setup!
    @klass.send :attr_accessor, :custom_attribute
  end
end

To test you can define a Validator class with the setup method defined and assert the deprecation message with the assert_deprecated method when calling validates_with. Something like this:

def test_setup_is_deprecated
  assert_deprecated do
    Class.new do
      include ActiveModel::Validations

      validates_with MyDeprecatedValidator
    end
  end
end
@apotonick
Copy link
Contributor Author

apotonick commented May 22, 2013

Now the last quest: Why is options.freeze called in Validator#initialize?

@rafaelfranca I wanted to say I really appreciate your responsiveness - amazing! 😸

@@ -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]

This comment has been minimized.

@josevalim

josevalim May 22, 2013 Contributor

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

@options = options.freeze
@klass = options[:class]
@options = options.except(:class).freeze
deprecated_setup or setup!

This comment has been minimized.

@josevalim

josevalim May 22, 2013 Contributor

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.

This comment has been minimized.

@josevalim

josevalim May 22, 2013 Contributor

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

This comment has been minimized.

@apotonick

apotonick May 22, 2013 Author Contributor

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]
@josevalim
Copy link
Contributor

josevalim commented May 22, 2013

I have added some comments, :+1 for getting rid of the respond_to? in general.

@apotonick
Copy link
Contributor Author

apotonick commented May 22, 2013

Cool! I removed the automatic setup! call, I personally like that better. Ready for merge!

def deprecated_setup(options) # 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:

This comment has been minimized.

@rafaelfranca

rafaelfranca May 22, 2013 Member

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

@rafaelfranca
Copy link
Member

rafaelfranca commented May 22, 2013

I like it too. Just a minor comment on the deprecation message.

@rafaelfranca
Copy link
Member

rafaelfranca commented May 22, 2013

Since it is a user facing change we will need a CHANGELOG entry, and make sure your commits are squashed.

@apotonick
Copy link
Contributor Author

apotonick commented May 23, 2013

Dunno how to change a PR to another branch, here it goes: https://github.com/apotonick/rails/tree/deprecate-validator-setup rebased to current rails/rails:master and including updated exception and CHANGELOG entry.

@rafaelfranca
Copy link
Member

rafaelfranca commented May 23, 2013

@apotonick merged.

Thank you for contributing.

@apotonick
Copy link
Contributor Author

apotonick commented May 23, 2013

Thank you @josevalim and especially @rafaelfranca for that quick and uncomplicated feedback and processing! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.