search

lcoq / ember-validations

Ember-Validations - An Ember.js library for handling object validations
MIT License
122 stars 25 forks source link

allowBlank option on numericality validator #11

Closed sohara closed 11 years ago

sohara commented 11 years ago

I don't intend this as a real pull request but I would like to get your feedback early on how best to implement this feature.

On the implementation: I've add a simple conditional on the validate method of Ember.Validators.NumericalityValidator. I'm not sure if this a too ugly way to do it? If I continue this way I would add this conditional to validate method of the other validators. Perhaps another way would be to implement this somehow in the parent Ember.Validator class. But this would meed the classes that extend Ember.Validator would need to all call _super() and then perhaps you lose the error handling to ensure they all have a validate function. Also, it doesn't make sense for the presence validator. Of course I'll modify the documentation for the real pull request.

On the test: It's the simplest test, just to ensure blank strings are allow through. But it passes and none of the other tests are failing as a result.

If you have a moment, please let me know your thoughts on the best approach and I'll go back and revise.

Thanks, Sean

lcoq commented 11 years ago

I think the best way to do this is to rename the validate methods of each validator: _validate. When someone wants to write a validator, he has to implement the _validate method. This method is private.

Then, each Ember.Validator should have a validate method that will call its shouldSkipValidations(object, attr, value) and will call _validate if the returned value is truthy. This method is firstly implemented in Ember.Validator. and just return false by default.

This implementation results in some changes, so if you won't have time for them, I will do it when possible. Of course, if you have any better idea or remark, I'll be glad to read it!

sohara commented 11 years ago

That sounds like a good way to do it. I'll work on it this evening or tomorrow and get you a new pull request.

sohara commented 11 years ago

Hi @lcoq I've add a couple of commits that change the implementation to what you suggested. I also added a note about skipping validations using the allowBlank option in the readme file. Let me know what you think. Sean

sohara commented 11 years ago

Also, let me know if you prefer that I rebase these changes into a single commit.

lcoq commented 11 years ago

I really like what you did, except for one thing: I told you the method shouldSkipValidations just return false by default. After reading your code, I believe it should return true when the allowBlank is true by default. So, except for the PresenceValidator, no one validator has to override the shouldSkipValidations. What do you think? Do you agree?

Anyway, thanks a lot for you time

sohara commented 11 years ago

Sure, I think the things you mentioned make sense. I will try to make these changes tomorrow and let you know when it has been updated. Thanks!

sohara commented 11 years ago

I've updated the pull request according to your suggestions. Let me know what you think! Thanks :)

lcoq commented 11 years ago

Merged in master. Thanks @sohara !

karlguillotte commented 11 years ago

Awesome, thanks guys! Nice addition to the API.