Added on option for validation matchers #14

Merged
merged 1 commit into from Aug 15, 2012

4 participants

@chancancode
Collaborator

No description provided.

@travisbot

This pull request passes (merged 0288812 into 76a476b).

@frodsan
Owner

Nice, could you add an entry to the CHANGELOG, please? :)

@travisbot

This pull request passes (merged c9973c36 into 76a476b).

@travisbot

This pull request passes (merged 24583c63 into 76a476b).

@chancancode
Collaborator

Done !

@travisbot

This pull request passes (merged 33b38335 into 76a476b).

@gr4y
Collaborator

@frodsan I vote for @chancancode getting contributor access too if he wants to! :)

@frodsan
Owner

👍 no problem, @chancancode ?

@frodsan frodsan and 1 other commented on an outdated diff Aug 15, 2012
lib/matchers/validations/validations.rb
@@ -34,18 +40,24 @@ def negative_failure_message
def description
desc = "validate #{@type.inspect} of #{@field.inspect}"
desc << " with message: #{@expected_message.inspect}" if @expected_message
+ desc << " on #{@expected_on.empty? ? 'all actions' : @expected_on.map(&:inspect).join(', ')}" if @expected_on
@frodsan
Owner
frodsan added a note Aug 15, 2012

Instead of:

@expected_on.map(&:inspect).join(', ')

You can use:

to_sentence(@expected_on)

Check: https://github.com/frodsan/mongoid-minitest/blob/master/lib/matchers/helpers.rb#L4

@chancancode
Collaborator

Good call!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@frodsan frodsan commented on an outdated diff Aug 15, 2012
lib/matchers/validations/validations.rb
@@ -60,6 +72,16 @@ def check_message
end
end
+ def check_on
+ on = clean_contexts(@validator.options[:on])
+ if on.sort == @expected_on.sort
+ @positive_message << " on #{on.empty? ? 'all actions' : on.map(&:inspect).join(', ')}"
@frodsan
Owner
frodsan added a note Aug 15, 2012

ditto.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@frodsan frodsan commented on an outdated diff Aug 15, 2012
lib/matchers/validations/validations.rb
@@ -60,6 +72,16 @@ def check_message
end
end
+ def check_on
+ on = clean_contexts(@validator.options[:on])
+ if on.sort == @expected_on.sort
+ @positive_message << " on #{on.empty? ? 'all actions' : on.map(&:inspect).join(', ')}"
+ else
+ @negative_message << " on #{on.empty? ? 'all actions' : on.map(&:inspect).join(', ')}"
@frodsan
Owner
frodsan added a note Aug 15, 2012

ditto.

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

This pull request passes (merged c851a06 into 76a476b).

@chancancode
Collaborator

Updated and squashed into one.

@frodsan About committer access, of course I wouldn't mind. But I just don't know if our view on the role of this project are completely aligned (for example, I'm using this gem to write assertion for my schema, i.e. the fact that this field exists and that validation rule is there, but I use bcardarella/valid_attribute to test the actually validation behaviour), so I'll probably still work off branches and send them as PR for code review for the time being.

@frodsan frodsan merged commit ba90173 into frodsan:master Aug 15, 2012

1 check passed

Details default The Travis build passed
@frodsan
Owner

@chancancode Hey mate, you have commit access now. I think valid_attribute gem is perfect if you want to test validations behaviour (valid?). I'm not a fan of those kind of tests, i prefer to test if the validation rules exists and trust in ActiveModel. I'm not currently working with mongoid, so thanks to both to keep this gem updated. I think there's a lot of space for improvement (i.e. docs).

Also, today i was thinking in extract the validation matchers in another gem and just make it a dependency. Basically, 'cause i would like to use the validation matchers with AR for example. I would like to hear your thoughts about this.

UPDATE: I know that there are some differences between some AR and Mongoid validations, but probably we can support both.

Thanks ❤️

@frodsan

@chancancode Mongoid's current version supports only +3.1 ActiveModel. Is this necessary?

Collaborator

The actually difference is this - in older version of AR (before validation got moved into AM), :on => could be either :create, :update or :save (which means :create + :update). At some point :save got dropped and it's recommended that if you want it to run on both :create and :update, you should just not touch the :on option as the default (nil) is to run on all validation contexts. I haven't done enough research on whether :on => :save is actually removed or just deprecated, and I thought it { must validate_presence_of(:field).on(nil) } sounds a bit strange, so I left it there and as a result you can do it { must validate_presence_of(:field).on(:save) } and it would work on either version of AR/AM

Owner

Ok, np :)

Collaborator

I'm a bit torn on this one either. If the AM recommendation is "if you want it to always run, stick with the default nil" then perhaps we shouldn't encourage people to write specs such as .on(:save) or .on(nil). On the other hand it could be argued that having tighter specs like this is a good idea to guard against future change in the defaults..

If someone could investigate and be 100% sure that .on(:save) is not supported on AM 3+, then I'm okay with removing that.

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