lukejagodzinski / meteor-astronomy-validators

https://atmospherejs.com/jagi/astronomy-validators
MIT License
11 stars 13 forks source link

maxLength should validate empty strings/arrays #11

Closed laurentpayot closed 8 years ago

laurentpayot commented 8 years ago

I couldn't use maxLength for a string that should be less than n characters but could eventually be empty. maxLength should return true for empty strings/arrays, as this validator should only check that a string/array is not too long.

lukejagodzinski commented 8 years ago

In deed there is a problem with using this validator with an empty string

if (!'') // true

however

if (![]) // false

I'm just wondering, if it's good to return true if a value evaluates to false. Maybe it's should be just:

if (_.isNull(fieldValue) || _.isUndefined(fieldValue) || !_.has(fieldValue, 'length')) {
  return false;
}

What do you think?

laurentpayot commented 8 years ago

I come from a python backgound and javascript is full of surprises (to say the less) when it comes to empty values evaluation. The solution you've just proposed looks fine to me :+1:

laurentpayot commented 8 years ago

fyi, a maxLength2 validator with your solution passed my unit tests.

lukejagodzinski commented 8 years ago

Ok great, I will make this fix tomorrow and publish it. However if you want to contribute, you can make a PR :)

laurentpayot commented 8 years ago

Ok, I created PR #12 for that...

lukejagodzinski commented 8 years ago

Ok thanks, I will merge it.