Skip to content

Conversation

@hellosylvee
Copy link

We TDDed out an implementation of the "presence" validator as described in issue #9.

We didn't look at the other validation factories, so this may not be aligned in terms of code style / patterns.
In particular, we noticed that the errors field of our validation result is an array and not a function like isValid.

(We also called our subject under test a factory, and not a validator ;))

@davwards
Copy link
Owner

Thanks y'all! Just a few points of order before we accept this:

  1. It looks like CI is blowing up on your Object.keys call; I suspect this is because CI is running node version 0.10.33, and you're running something more recent locally. I need to talk with @mattrothenberg about whether we want to prescribe a version of Node, if so, which one we should pick, and how to ensure that contributors don't get surprised by version differences.

However, we can sidestep the whole issue for this pull request—I think the least surprising thing would be for empty objects to pass the presence validator. E.g.,

Validation().present().validate({}).isValid()

should evaluate to true. If users need to ensure that objects are not empty, they'll be able to use your validator with a forAttribute modifier, or as part of an object schema, to specify the object attributes they're looking for.

  1. There are helper modules for creating Result objects that validators should use instead of building result objects by hand. That way, in the event we need to change something about Result objects (which we've had to do before), we can change their constructors without having to update all the validators. I'll make a change to the README to make that more obvious! So you'll want to require these modules:
results/invalid.js
results/valid.js

and use the functions they export to build your result object, instead of what you have here:

return {
  isValid: valid,
  errors: errors
};
  1. It looks like this implementation invalidates numerical values. E.g.:
var totalPrice = 10;
Validation().present().validate(totalPrice).isValid() // false, but ought to be true!

If you could push up fixes for those three issues, we'll get you merged!

@mattrothenberg
Copy link
Collaborator

@davwards That's an interesting question about locking down the Node version. My gut says to take another stab at the validator code, replacing the Object.keys call with something different (that both handles the validation logic correctly and doesn't upset Node). If we start running into more scenarios where our logic is causing Node explosions, we should consider prescribing a version. Thoughts?

P.S: @davwards Are empty arrays [] considered valid, too?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants