defined validation #110

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
2 participants
@ajselvig
Contributor

ajselvig commented Dec 20, 2014

Okay, tear it apart :)

lib/no_brainer/document/defined.rb
+
+ included do
+ singleton_class.send(:attr_accessor, :defined_validators)
+ self.defined_validators = []

This comment has been minimized.

@nviennot

nviennot Dec 20, 2014

Owner

Why do you maintain such defined_validators array? You are not reading from it anywhere

@nviennot

nviennot Dec 20, 2014

Owner

Why do you maintain such defined_validators array? You are not reading from it anywhere

This comment has been minimized.

@ajselvig

ajselvig Dec 22, 2014

Contributor

To be honest, this was copied verbatim from the uniqueness validator without complete understanding of what it did. I assumed it passed the validations on to subclasses.

@ajselvig

ajselvig Dec 22, 2014

Contributor

To be honest, this was copied verbatim from the uniqueness validator without complete understanding of what it did. I assumed it passed the validations on to subclasses.

lib/no_brainer/document/defined.rb
+ end
+
+ def validate_each(doc, attr, value)
+ if value.nil?

This comment has been minimized.

@nviennot

nviennot Dec 20, 2014

Owner

validate_each() no longer need to have a return value, no need for the true/false thingy

@nviennot

nviennot Dec 20, 2014

Owner

validate_each() no longer need to have a return value, no need for the true/false thingy

lib/no_brainer/document/defined.rb
+ end
+ end
+ end
+end

This comment has been minimized.

@nviennot

nviennot Dec 20, 2014

Owner

Missing a new line

@nviennot

nviennot Dec 20, 2014

Owner

Missing a new line

lib/no_brainer/document/defined.rb
@@ -0,0 +1,41 @@
+module NoBrainer::Document::Defined

This comment has been minimized.

@nviennot

nviennot Dec 20, 2014

Owner

let's call the validator definition so we can do validates_definition_of ...
This seems pretty natural.

@nviennot

nviennot Dec 20, 2014

Owner

let's call the validator definition so we can do validates_definition_of ...
This seems pretty natural.

lib/no_brainer/document/validation.rb
@@ -25,6 +25,7 @@ def _field(attr, options={})
super
validates(attr, :format => { :with => options[:format] }) if options.has_key?(:format)
validates(attr, :presence => options[:required]) if options.has_key?(:required)

This comment has been minimized.

@nviennot

nviennot Dec 20, 2014

Owner

required should be a shortcut for the definition validator
presence should be a shortcut for the presence validator

@nviennot

nviennot Dec 20, 2014

Owner

required should be a shortcut for the definition validator
presence should be a shortcut for the presence validator

This comment has been minimized.

@nviennot

nviennot Dec 20, 2014

Owner

Actually, required => true should use the definition validator, and :required => :presence should use the presence validator.

@nviennot

nviennot Dec 20, 2014

Owner

Actually, required => true should use the definition validator, and :required => :presence should use the presence validator.

lib/no_brainer/document/defined.rb
+
+ def validate_each(doc, attr, value)
+ if value.nil?
+ doc.errors.add(attr, 'must be defined')

This comment has been minimized.

@nviennot

nviennot Dec 20, 2014

Owner

Use doc.errors.add(attr, :undefined, options) and define the error message in lib/nobrainer/locale/en.yml (for i18n).

@nviennot

nviennot Dec 20, 2014

Owner

Use doc.errors.add(attr, :undefined, options) and define the error message in lib/nobrainer/locale/en.yml (for i18n).

spec/defined_spec.rb
+
+ it 'cannot save without setting a value' do
+ doc = SimpleDocument.new
+ doc.valid?.should == false

This comment has been minimized.

@nviennot

nviennot Dec 20, 2014

Owner

also test that doc.errors.full_messages.first is Field1 must be defined

@nviennot

nviennot Dec 20, 2014

Owner

also test that doc.errors.full_messages.first is Field1 must be defined

spec/defined_spec.rb
@@ -0,0 +1,36 @@
+require 'spec_helper'
+
+describe 'NoBrainer callbacks' do

This comment has been minimized.

@nviennot

nviennot Dec 20, 2014

Owner

callbacks -> validation

@nviennot

nviennot Dec 20, 2014

Owner

callbacks -> validation

spec/defined_spec.rb
+ doc.valid?.should == true
+ end
+ end
+end

This comment has been minimized.

@nviennot

nviennot Dec 20, 2014

Owner

missing newline

@nviennot

nviennot Dec 20, 2014

Owner

missing newline

spec/defined_spec.rb
+ end
+
+ it 'cannot save a nil value' do
+ doc = SimpleDocument.new field1: nil

This comment has been minimized.

@nviennot

nviennot Dec 20, 2014

Owner
  1. Don't use the a: b syntax, but the a => b syntax (everywhere).
  2. Use parenthesis
@nviennot

nviennot Dec 20, 2014

Owner
  1. Don't use the a: b syntax, but the a => b syntax (everywhere).
  2. Use parenthesis

This comment has been minimized.

@ajselvig

ajselvig Dec 22, 2014

Contributor

I applaud your commitment to Ruby 1.8, but what's the deal with the parentheses?

@ajselvig

ajselvig Dec 22, 2014

Contributor

I applaud your commitment to Ruby 1.8, but what's the deal with the parentheses?

This comment has been minimized.

@nviennot

nviennot Dec 23, 2014

Owner
  • Always use the => notation for hashes, because the : notation works only with symbols, but not with variables, and I don't want to mix two different styles
  • Always use parentheses when you have a function call with at least one argument. Don't use parentheses for anything that looks like a keyword in DSLs (e.g. field :name).
@nviennot

nviennot Dec 23, 2014

Owner
  • Always use the => notation for hashes, because the : notation works only with symbols, but not with variables, and I don't want to mix two different styles
  • Always use parentheses when you have a function call with at least one argument. Don't use parentheses for anything that looks like a keyword in DSLs (e.g. field :name).
@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Dec 22, 2014

Owner

BTW the pull request has a bunch of comments if you wish to revise it

Owner

nviennot commented Dec 22, 2014

BTW the pull request has a bunch of comments if you wish to revise it

@ajselvig

This comment has been minimized.

Show comment
Hide comment
@ajselvig

ajselvig Dec 22, 2014

Contributor

Yes yes, I'm getting there :)

On Mon, Dec 22, 2014 at 3:32 PM, Nicolas Viennot notifications@github.com
wrote:

BTW the pull request has a bunch of comments if you wish to revise it


Reply to this email directly or view it on GitHub
#110 (comment).

Contributor

ajselvig commented Dec 22, 2014

Yes yes, I'm getting there :)

On Mon, Dec 22, 2014 at 3:32 PM, Nicolas Viennot notifications@github.com
wrote:

BTW the pull request has a bunch of comments if you wish to revise it


Reply to this email directly or view it on GitHub
#110 (comment).

lib/no_brainer/document/definition.rb
+ extend ActiveSupport::Concern
+
+ included do
+ singleton_class.send(:attr_accessor, :defined_validators)

This comment has been minimized.

@nviennot

nviennot Dec 23, 2014

Owner

Remove all these lines, you don't need to maintain a "defined_validators"

@nviennot

nviennot Dec 23, 2014

Owner

Remove all these lines, you don't need to maintain a "defined_validators"

lib/no_brainer/document/validation.rb
@@ -24,7 +24,8 @@ module ClassMethods
def _field(attr, options={})
super
validates(attr, :format => { :with => options[:format] }) if options.has_key?(:format)
- validates(attr, :presence => options[:required]) if options.has_key?(:required)
+ validates(attr, :presence => true) if options.has_key?(:required) && options[:required]==:presence

This comment has been minimized.

@nviennot

nviennot Dec 23, 2014

Owner

use a case options[:required]

@nviennot

nviennot Dec 23, 2014

Owner

use a case options[:required]

added some parentheses, removed unnecessary definition_validators cod…
…e, changed required options check to a case statement
lib/no_brainer/document/definition.rb
+ end
+
+ class DefinitionValidator < ActiveModel::EachValidator
+ attr_accessor :scope

This comment has been minimized.

@nviennot

nviennot Dec 24, 2014

Owner

what's that for?
you can probably remove that :)

@nviennot

nviennot Dec 24, 2014

Owner

what's that for?
you can probably remove that :)

lib/no_brainer/document/validation.rb
+
+ # required validations
+ case options[:required]
+ when :presence

This comment has been minimized.

@nviennot

nviennot Dec 24, 2014

Owner

Use oneliner for the when clauses. example:

case options[:required]
when :presence then validates(...)
when :definition, true then validates(...)
when nil then ;
else raise "XXX"
end

👍 on the raise.

@nviennot

nviennot Dec 24, 2014

Owner

Use oneliner for the when clauses. example:

case options[:required]
when :presence then validates(...)
when :definition, true then validates(...)
when nil then ;
else raise "XXX"
end

👍 on the raise.

This comment has been minimized.

@nviennot

nviennot Dec 24, 2014

Owner

Also, the case and when should have the same indentation level

@nviennot

nviennot Dec 24, 2014

Owner

Also, the case and when should have the same indentation level

lib/no_brainer/document/validation.rb
validates(attr, :uniqueness => options[:unique]) if options.has_key?(:unique)
validates(attr, :uniqueness => options[:uniq]) if options.has_key?(:uniq)
validates(attr, :inclusion => {:in => options[:in]}) if options.has_key?(:in)
validates(attr, options[:validates]) if options[:validates]
+
+ # required validations

This comment has been minimized.

@nviennot

nviennot Dec 24, 2014

Owner

No need for that comment, the code is as readable as this comment. Comment does not add anything of value here.

@nviennot

nviennot Dec 24, 2014

Owner

No need for that comment, the code is as readable as this comment. Comment does not add anything of value here.

spec/integration/definition_spec.rb
+ doc = SimpleDocument.new
+ doc.valid?.should == false
+ expect { doc.save }.to raise_error(NoBrainer::Error::DocumentInvalid)
+ doc.errors.full_messages.first == 'Field1 must be defined'

This comment has been minimized.

@nviennot

nviennot Dec 24, 2014

Owner

You are missing a .should -- otherwise this statement does not test anything.

@nviennot

nviennot Dec 24, 2014

Owner

You are missing a .should -- otherwise this statement does not test anything.

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Dec 24, 2014

Owner

It's getting there :) :)

Owner

nviennot commented Dec 24, 2014

It's getting there :) :)

+ doc = SimpleDocument.new(:field1 => [])
+ doc.valid?.should == true
+ end
+ end

This comment has been minimized.

@nviennot

nviennot Dec 24, 2014

Owner

Test also the :required shortcuts for field definitions. The way you are writing your tests is good. Right now, you are using a context context 'with validates_definition_of' do, you might want to use 2 different contexts for what you are doing. true and :presence. Ideally we want to also test for :definition and something invalid to test for all the case paths in the.

You can also remove this test once done: https://github.com/nviennot/nobrainer/blob/master/spec/integration/validation_spec.rb#L158-L165

@nviennot

nviennot Dec 24, 2014

Owner

Test also the :required shortcuts for field definitions. The way you are writing your tests is good. Right now, you are using a context context 'with validates_definition_of' do, you might want to use 2 different contexts for what you are doing. true and :presence. Ideally we want to also test for :definition and something invalid to test for all the case paths in the.

You can also remove this test once done: https://github.com/nviennot/nobrainer/blob/master/spec/integration/validation_spec.rb#L158-L165

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Dec 30, 2014

Owner

ping?

Owner

nviennot commented Dec 30, 2014

ping?

@ajselvig

This comment has been minimized.

Show comment
Hide comment
@ajselvig

ajselvig Dec 31, 2014

Contributor

Been traveling the last couple weekends and pretty busy with our annual
transition at work (you don't even want to know). I should get to this
shortly.

I will say, if you're going to reject pull requests for trivial syntax
(case formatting, presence of comments), it would be helpful for
contributors to have a style guide so we don't have to spend so many
iterations on this sort of thing :)

On Tue, Dec 30, 2014 at 1:54 PM, Nicolas Viennot notifications@github.com
wrote:

ping?


Reply to this email directly or view it on GitHub
#110 (comment).

Contributor

ajselvig commented Dec 31, 2014

Been traveling the last couple weekends and pretty busy with our annual
transition at work (you don't even want to know). I should get to this
shortly.

I will say, if you're going to reject pull requests for trivial syntax
(case formatting, presence of comments), it would be helpful for
contributors to have a style guide so we don't have to spend so many
iterations on this sort of thing :)

On Tue, Dec 30, 2014 at 1:54 PM, Nicolas Viennot notifications@github.com
wrote:

ping?


Reply to this email directly or view it on GitHub
#110 (comment).

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Jan 1, 2015

Owner

Thank you Andy :)

I've made a couple of changes from your original PR:

  1. Renamed "defined" to "not_null" -- it's a little more explicit of what it's doing (technically, defined should accept nil, if explicitly set)
  2. Removed the :required => :presence and all that. The presence validator is nonsense anyway, length validator should be used in most cases, for which I added a shorthand.
  3. Refactored the tests with shared examples: https://github.com/nviennot/nobrainer/blob/master/spec/integration/validation/definition_spec.rb#L69-L82

That's all,
Happy new year :)

Owner

nviennot commented Jan 1, 2015

Thank you Andy :)

I've made a couple of changes from your original PR:

  1. Renamed "defined" to "not_null" -- it's a little more explicit of what it's doing (technically, defined should accept nil, if explicitly set)
  2. Removed the :required => :presence and all that. The presence validator is nonsense anyway, length validator should be used in most cases, for which I added a shorthand.
  3. Refactored the tests with shared examples: https://github.com/nviennot/nobrainer/blob/master/spec/integration/validation/definition_spec.rb#L69-L82

That's all,
Happy new year :)

@ajselvig

This comment has been minimized.

Show comment
Hide comment
@ajselvig

ajselvig Jan 1, 2015

Contributor

Okay, sounds good. Happy new year as well!

On Thu, Jan 1, 2015 at 4:26 PM, Nicolas Viennot notifications@github.com
wrote:

Thank you Andy :)

I've made a couple of changes from your original PR:

  1. Renamed "defined" to "not_null" -- it's a little more explicit of what
    it's doing (technically, defined should accept nil, if explicitly set)
  2. Removed the :required => :presence and all that. The presence validator
    is nonsense anyway, length validator should be used in most cases, for
    which I added a shorthand.
  3. Refactored the tests with shared examples:
    https://github.com/nviennot/nobrainer/blob/master/spec/integration/validation/definition_spec.rb#L69-L82

That's all,
Happy new year :)


Reply to this email directly or view it on GitHub
#110 (comment).

Contributor

ajselvig commented Jan 1, 2015

Okay, sounds good. Happy new year as well!

On Thu, Jan 1, 2015 at 4:26 PM, Nicolas Viennot notifications@github.com
wrote:

Thank you Andy :)

I've made a couple of changes from your original PR:

  1. Renamed "defined" to "not_null" -- it's a little more explicit of what
    it's doing (technically, defined should accept nil, if explicitly set)
  2. Removed the :required => :presence and all that. The presence validator
    is nonsense anyway, length validator should be used in most cases, for
    which I added a shorthand.
  3. Refactored the tests with shared examples:
    https://github.com/nviennot/nobrainer/blob/master/spec/integration/validation/definition_spec.rb#L69-L82

That's all,
Happy new year :)


Reply to this email directly or view it on GitHub
#110 (comment).

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