Accept a symbol for `:in` option on inclusion and exclusion validators #7342

Merged
merged 1 commit into from Aug 24, 2012

Projects

None yet

7 participants

Contributor

I'm not sure about my english on documentation but the feature is fine :)

I was in doubt about this line:

unless [:include?, :call].any?{ |method| delimiter.respond_to?(method) } || delimiter.is_a?(Symbol)

It may be more clear about what is happening, but this is not a big deal:

unless delimiter.respond_to?(:include?) || delimiter.respond_to?(:call) || delimiter.is_a?(Symbol)

What do you think?

Owner

Thank you for the pull request.

Could you explain what are the advantages over the proc/lambda?

Contributor

@rafaelfranca I have a model called Revenue, other called Payment and other called PaymentRevenue.

I must validate if the revenue of payment revenue is a child revenue of payment revenue.

Like that:

class Revenue < ActiveRecord::Base
  belongs_to :parent, :foreign_key => :parent_id
  has_many :children, :foreign_key => :parent_id
end
class Payment < ActiveRecord::Base
  belongs_to :revenue

  has_many :payment_revenue
end
class PaymentRevenue < ActiveRecord::Base
  belongs_to :payment
  belongs_to :revenue

  validates :revenues, :inclusion => :available_revenues

  def available_revenues
    payment.revenue.children
  end
end

I have this revenue tree:

IPTU
- Imposto Predial
- Imposto Territorial
- Taxa de Expediente

And I have a payment like this one:

Pagamento - IPTU

Imposto Predial - R$ 50,00
Imposto Territorial - R$ 50,00
Taxa de Expediente - R$ 50,00

I can't pick a revenue that is not associated to payment revenue, IPTU in this case :)

Owner

But I think you can do the same thing using proc/lambda

class PaymentRevenue < ActiveRecord::Base
  belongs_to :payment
  belongs_to :revenue

  validates :revenues, inclusion: { in: -> { |record| record.available_revenues } }

  def available_revenues
    payment.revenue.children
  end
end

I'm not against neither in favor. I'm just want to know if this is adding feature or is only syntax sugar.

Contributor

@rafaelfranca nope.

I don't want to make this kind of method public on my objects.

This code:

class PaymentRevenue < ActiveRecord::Base
  belongs_to :payment
  belongs_to :revenue

  validates :revenue, :inclusion => { :in => lambda { |record| record.available_revenues } }

  protected

  def available_revenues
    payment.revenue.children
  end
end

Raises an exception:

NoMethodError: protected method `available_revenues' called for #<PaymentRevenue:0x007f93126a50e8>

Syntax suggar is a side effect :)

Owner

Got it. Thank you to explain.

@rafaelfranca rafaelfranca and 2 others commented on an outdated diff Aug 12, 2012
activemodel/CHANGELOG.md
@@ -1,5 +1,18 @@
## Rails 4.0.0 (unreleased) ##
+* Changed inclusion and exclusion validators to accept a symbol for `:in` option.
+ This allow to use dynamic inclusion/exclusion values, i.e.:
rafaelfranca
rafaelfranca Aug 12, 2012 Owner

Maybe this line is not correct since we already can use dynamic inclusion/exclusion values using proc/lambda.

sobrinho
sobrinho Aug 12, 2012 Contributor

@rafaelfranca should I change to

Changed inclusion and exclusion validators to accept a symbol for `:in` option, i.e.:

?

carlosantoniodasilva
carlosantoniodasilva Aug 12, 2012 Owner

Perhaps just an addition? This allows to use dynamic inclusion/exclusion values using methods, besides the current lambda/proc support.

@rafaelfranca rafaelfranca commented on an outdated diff Aug 12, 2012
activemodel/lib/active_model/validations/clusivity.rb
@@ -3,11 +3,11 @@
module ActiveModel
module Validations
module Clusivity #:nodoc:
- ERROR_MESSAGE = "An object with the method #include? or a proc or lambda is required, " <<
+ ERROR_MESSAGE = "An object with the method #include? or a proc or lambda or symbol is required, " <<
rafaelfranca
rafaelfranca Aug 12, 2012 Owner

An object with the method #include? or a proc, lambda or symbol is required

@rafaelfranca rafaelfranca commented on an outdated diff Aug 12, 2012
activemodel/lib/active_model/validations/clusivity.rb
"and must be supplied as the :in (or :within) option of the configuration hash"
def check_validity!
- unless [:include?, :call].any?{ |method| delimiter.respond_to?(method) }
+ unless [:include?, :call].any?{ |method| delimiter.respond_to?(method) } || delimiter.is_a?(Symbol)
rafaelfranca
rafaelfranca Aug 12, 2012 Owner

Yes, I prefer your suggestion here.

Contributor

@rafaelfranca should I make new commits or make an amend and push using force?

I don't want to make this kind of method public on my objects.

Technically you're just hiding the send call inside the validation, rather than making it explicit:

  validates :revenue, :inclusion => { :in => ->(r) { r.send(:available_revenues) } }

  protected

  def available_revenues
    payment.revenue.children
  end

Just my two cents :), thanks mate.

Member
sikachu commented Aug 12, 2012

Use the force, Luke.

Contributor

@carlosantoniodasilva looking in this way, I'm doing just a syntax sugar :)

Contributor

@sikachu LOOOOL

Contributor

@rafaelfranca I've updated the pull request.

You could merge that and correct the CHANGELOG or I can do that here, but I'm not sure what to write :)

Contributor

@carlosantoniodasilva @rafaelfranca I've updated the changelog.

Could you review again? :)

Contributor
nthj commented Aug 24, 2012

This makes sense because rails uses this approach for other callbacks, i.e. before_save :notify_everyone, :if => :name_changed? I see validations as a special case of before_save callbacks, so equivalent functionality would follow from that premise. 👍

Member

It'll need a rebase. (damn changelogs)

Contributor

@steveklabnik I will do that, wait a while :)

Contributor

@steveklabnik done! :)

@rafaelfranca rafaelfranca commented on an outdated diff Aug 24, 2012
activemodel/lib/active_model/validations/clusivity.rb
@@ -15,7 +15,14 @@ def check_validity!
private
def include?(record, value)
- exclusions = delimiter.respond_to?(:call) ? delimiter.call(record) : delimiter
+ exclusions = if delimiter.respond_to?(:call)
+ delimiter.call(record)
+ elsif delimiter.is_a?(Symbol)
rafaelfranca
rafaelfranca Aug 24, 2012 Owner

Maybe a respond_to?(:to_sym) check is better here.

Contributor

@rafaelfranca done! :)

@rafaelfranca rafaelfranca and 1 other commented on an outdated diff Aug 24, 2012
activemodel/lib/active_model/validations/clusivity.rb
"and must be supplied as the :in (or :within) option of the configuration hash"
def check_validity!
- unless [:include?, :call].any?{ |method| delimiter.respond_to?(method) }
+ unless delimiter.respond_to?(:include?) || delimiter.respond_to?(:call) || delimiter.is_a?(Symbol)
rafaelfranca
rafaelfranca Aug 24, 2012 Owner

You forgot here.

unless [:include?, :call, :to_sym].any?{ |method| delimiter.respond_to?(method) }
sobrinho
sobrinho Aug 24, 2012 Contributor

@rafaelfranca do you prefer an array containing the methods?

I don't feel good making a hash with 3 symbols and calling the any? and after that checking the method.

I think it's too much computation for a simple thing.

Anyway, that is not a "performance issue spot", I just want to be sure before changing :)

rafaelfranca
rafaelfranca Aug 24, 2012 Owner

I'm fine with the || too.

Contributor

@rafaelfranca done! :)

@rafaelfranca rafaelfranca merged commit 35cb772 into rails:master Aug 24, 2012
Owner

Merged. Thanks

why not do so?
[:include?, :call, :to_sym].any?{ |method| delimiter.respond_to?(method) }

Owner

We prefer this way to avoid array manipulation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment