search

jbrunton / frappuccino-core

2 stars 3 forks source link

Validator cleanup, additions, tests #23

Open dklon opened 11 years ago

dklon commented 11 years ago

New numericality validator Specs for all validators Added initialize_attribute method to base_validator to copy attributes from opts

jbrunton commented 11 years ago

According to Travis, the jasmine stories are failing (see above note from Travis on the pull request). On occasion, this can be due to a difference in environments, but if this command fails for you can you fix and resubmit?

bundle exec guard-jasmine -u http://localhost:3001/jasmine-stories
dklon commented 11 years ago

Fixed presence_validator.js so that now "bundle exec guard-jasmine -u http://localhost:3001/jasmine-stories" is passing.

Basically I'd introduced an initialize_attributes method to the BaseValidator class so that attributes can be easily copied from the opts to the validator. For some reason using the initialize_attributes method in the PresenceValidator was causing a problem (fine in all the other validators though).

I played around with it a lot but was unable to understand why this was a problem with this class (setting the opts to {}, null, undefined in the PresenceValidator constructor meant the jasmine-stories went through fine). Wondering if it's something to do with how you setup the jasmine-stories which is why I don't quite understand what the problem is? Would you mind taking a look and explaining to me why it was complaining when PresenceValidator was using initialize_attributes but not when any of the other validators were?

For now I've reverted it back to it's previous state (@message = opts?.message) which leaves the jasmine stories happy.

jbrunton commented 11 years ago

It arises because not all validators expect to be provided a JSON object as the opts param.

Here are two equally valid calls to validate; the first accepts a JSON object:

@validates "user_name", length: { min: 5 }

but simpler validators might not accept any "options", so are enabled just with a boolean flag:

@validates "user_name", presence: true

The implementation of initialize_attributes fails in this case. I'd suggest modifying it to first check if the opts param is an object (Underscore provides a method for this), and only copying the fields if it is.

The fact that no unit tests picked up on this suggests we could use some tests for the BaseValidator class and this new method. I note that I failed to provide any for the class at all – maybe you'd like to provide a new test for your new method, and I'll fill in the remainder when I merge in your changes.