Skip to content
This repository

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

Merged
merged 1 commit into from over 1 year ago

7 participants

Gabriel Sobrinho Rafael Mendonça França Carlos Antonio da Silva Prem Sichanugrist Nathaniel Jones Steve Klabnik Adimir Colen
Gabriel Sobrinho

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?

Rafael Mendonça França
Owner

Thank you for the pull request.

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

Gabriel Sobrinho

@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 :)

Rafael Mendonça França
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.

Gabriel Sobrinho

@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 :)

Rafael Mendonça França
Owner

Got it. Thank you to explain.

activemodel/CHANGELOG.md
... ...
@@ -1,5 +1,18 @@
1 1
 ## Rails 4.0.0 (unreleased) ##
2 2
 
  3
+*   Changed inclusion and exclusion validators to accept a symbol for `:in` option.
  4
+    This allow to use dynamic inclusion/exclusion values, i.e.:
3
Rafael Mendonça França Owner

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

Gabriel Sobrinho
sobrinho added a note August 12, 2012

@rafaelfranca should I change to

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

?

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
activemodel/lib/active_model/validations/clusivity.rb
... ...
@@ -3,11 +3,11 @@
3 3
 module ActiveModel
4 4
   module Validations
5 5
     module Clusivity #:nodoc:
6  
-      ERROR_MESSAGE = "An object with the method #include? or a proc or lambda is required, " <<
  6
+      ERROR_MESSAGE = "An object with the method #include? or a proc or lambda or symbol is required, " <<
1
Rafael Mendonça França Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
activemodel/lib/active_model/validations/clusivity.rb
((6 lines not shown))
7 7
                       "and must be supplied as the :in (or :within) option of the configuration hash"
8 8
 
9 9
       def check_validity!
10  
-        unless [:include?, :call].any?{ |method| delimiter.respond_to?(method) }
  10
+        unless [:include?, :call].any?{ |method| delimiter.respond_to?(method) } || delimiter.is_a?(Symbol)
1
Rafael Mendonça França Owner

Yes, I prefer your suggestion here.

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

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

Carlos Antonio da Silva
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.

Prem Sichanugrist
Collaborator

Use the force, Luke.

Gabriel Sobrinho

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

Gabriel Sobrinho

@sikachu LOOOOL

Gabriel Sobrinho

@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 :)

Gabriel Sobrinho

@carlosantoniodasilva @rafaelfranca I've updated the changelog.

Could you review again? :)

Nathaniel Jones
nthj commented August 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. :+1:

Steve Klabnik
Collaborator

It'll need a rebase. (damn changelogs)

Gabriel Sobrinho

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

activemodel/lib/active_model/validations/clusivity.rb
... ...
@@ -15,7 +15,14 @@ def check_validity!
15 15
     private
16 16
 
17 17
       def include?(record, value)
18  
-        exclusions = delimiter.respond_to?(:call) ? delimiter.call(record) : delimiter
  18
+        exclusions = if delimiter.respond_to?(:call)
  19
+                       delimiter.call(record)
  20
+                     elsif delimiter.is_a?(Symbol)
1
Rafael Mendonça França Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
activemodel/lib/active_model/validations/clusivity.rb
((6 lines not shown))
7 7
                       "and must be supplied as the :in (or :within) option of the configuration hash"
8 8
 
9 9
       def check_validity!
10  
-        unless [:include?, :call].any?{ |method| delimiter.respond_to?(method) }
  10
+        unless delimiter.respond_to?(:include?) || delimiter.respond_to?(:call) || delimiter.is_a?(Symbol)
3
Rafael Mendonça França Owner

You forgot here.

unless [:include?, :call, :to_sym].any?{ |method| delimiter.respond_to?(method) }
Gabriel Sobrinho
sobrinho added a note August 24, 2012

@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 :)

Rafael Mendonça França Owner

I'm fine with the || too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Rafael Mendonça França rafaelfranca merged commit 35cb772 into from August 24, 2012
Rafael Mendonça França rafaelfranca closed this August 24, 2012
Rafael Mendonça França
Owner

Merged. Thanks

Adimir Colen

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

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
This page is out of date. Refresh to see the latest.
6  activemodel/CHANGELOG.md
Source Rendered
... ...
@@ -1,5 +1,11 @@
1 1
 ## Rails 4.0.0 (unreleased) ##
2 2
 
  3
+*   Changed inclusion and exclusion validators to accept a symbol for `:in` option.
  4
+
  5
+    This allows to use dynamic inclusion/exclusion values using methods, besides the current lambda/proc support.
  6
+
  7
+    *Gabriel Sobrinho*
  8
+
3 9
 *   `AM::Validation#validates` ability to pass custom exception to `:strict` option.
4 10
 
5 11
     *Bogdan Gusiev*
13  activemodel/lib/active_model/validations/clusivity.rb
@@ -3,11 +3,11 @@
3 3
 module ActiveModel
4 4
   module Validations
5 5
     module Clusivity #:nodoc:
6  
-      ERROR_MESSAGE = "An object with the method #include? or a proc or lambda is required, " <<
  6
+      ERROR_MESSAGE = "An object with the method #include? or a proc, lambda or symbol is required, " <<
7 7
                       "and must be supplied as the :in (or :within) option of the configuration hash"
8 8
 
9 9
       def check_validity!
10  
-        unless [:include?, :call].any?{ |method| delimiter.respond_to?(method) }
  10
+        unless delimiter.respond_to?(:include?) || delimiter.respond_to?(:call) || delimiter.respond_to?(:to_sym)
11 11
           raise ArgumentError, ERROR_MESSAGE
12 12
         end
13 13
       end
@@ -15,7 +15,14 @@ def check_validity!
15 15
     private
16 16
 
17 17
       def include?(record, value)
18  
-        exclusions = delimiter.respond_to?(:call) ? delimiter.call(record) : delimiter
  18
+        exclusions = if delimiter.respond_to?(:call)
  19
+                       delimiter.call(record)
  20
+                     elsif delimiter.respond_to?(:to_sym)
  21
+                       record.send(delimiter)
  22
+                     else
  23
+                       delimiter
  24
+                     end
  25
+
19 26
         exclusions.send(inclusion_method(exclusions), value)
20 27
       end
21 28
 
3  activemodel/lib/active_model/validations/exclusion.rb
@@ -24,11 +24,12 @@ module HelperMethods
24 24
       #     validates_exclusion_of :format, in: %w( mov avi ), message: "extension %{value} is not allowed"
25 25
       #     validates_exclusion_of :password, in: ->(person) { [person.username, person.first_name] },
26 26
       #                            message: 'should not be the same as your username or first name'
  27
+      #     validates_exclusion_of :karma, in: :reserved_karmas
27 28
       #   end
28 29
       #
29 30
       # Configuration options:
30 31
       # * <tt>:in</tt> - An enumerable object of items that the value shouldn't
31  
-      #   be part of. This can be supplied as a proc or lambda which returns an
  32
+      #   be part of. This can be supplied as a proc, lambda or symbol which returns an
32 33
       #   enumerable. If the enumerable is a range the test is performed with
33 34
       # * <tt>:within</tt> - A synonym(or alias) for <tt>:in</tt>
34 35
       #   <tt>Range#cover?</tt>, otherwise with <tt>include?</tt>.
3  activemodel/lib/active_model/validations/inclusion.rb
@@ -23,11 +23,12 @@ module HelperMethods
23 23
       #     validates_inclusion_of :age, in: 0..99
24 24
       #     validates_inclusion_of :format, in: %w( jpg gif png ), message: "extension %{value} is not included in the list"
25 25
       #     validates_inclusion_of :states, in: ->(person) { STATES[person.country] }
  26
+      #     validates_inclusion_of :karma, in: :available_karmas
26 27
       #   end
27 28
       #
28 29
       # Configuration options:
29 30
       # * <tt>:in</tt> - An enumerable object of available items. This can be
30  
-      #   supplied as a proc or lambda which returns an enumerable. If the
  31
+      #   supplied as a proc, lambda or symbol which returns an enumerable. If the
31 32
       #   enumerable is a range the test is performed with <tt>Range#cover?</tt>,
32 33
       #   otherwise with <tt>include?</tt>.
33 34
       # * <tt>:within</tt> - A synonym(or alias) for <tt>:in</tt>
22  activemodel/test/cases/validations/exclusion_validation_test.rb
@@ -64,4 +64,26 @@ def test_validates_exclusion_of_with_lambda
64 64
     t.title = "wasabi"
65 65
     assert t.valid?
66 66
   end
  67
+
  68
+  def test_validates_inclusion_of_with_symbol
  69
+    Person.validates_exclusion_of :karma, :in => :reserved_karmas
  70
+
  71
+    p = Person.new
  72
+    p.karma = "abe"
  73
+
  74
+    def p.reserved_karmas
  75
+      %w(abe)
  76
+    end
  77
+
  78
+    assert p.invalid?
  79
+    assert_equal ["is reserved"], p.errors[:karma]
  80
+
  81
+    def p.reserved_karmas
  82
+      %w()
  83
+    end
  84
+
  85
+    assert p.valid?
  86
+  ensure
  87
+    Person.reset_callbacks(:validate)
  88
+  end
67 89
 end
22  activemodel/test/cases/validations/inclusion_validation_test.rb
@@ -96,4 +96,26 @@ def test_validates_inclusion_of_with_lambda
96 96
     t.title = "elephant"
97 97
     assert t.valid?
98 98
   end
  99
+
  100
+  def test_validates_inclusion_of_with_symbol
  101
+    Person.validates_inclusion_of :karma, :in => :available_karmas
  102
+
  103
+    p = Person.new
  104
+    p.karma = "Lifo"
  105
+
  106
+    def p.available_karmas
  107
+      %w()
  108
+    end
  109
+
  110
+    assert p.invalid?
  111
+    assert_equal ["is not included in the list"], p.errors[:karma]
  112
+
  113
+    def p.available_karmas
  114
+      %w(Lifo)
  115
+    end
  116
+
  117
+    assert p.valid?
  118
+  ensure
  119
+    Person.reset_callbacks(:validate)
  120
+  end
99 121
 end
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.