-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Validation groups #15
Comments
Hmm I don't know if I understand you correctly, but if the field's value is not a nested object you want the validator to look for the given property in entire "parent" object? User = Astro.Class({
/* ... */
fields: {
address: 'object',
validators: Validators.has('city')
}
});
var user = new User();
user.validate('address'); And now, if the If it goes about second question. It's no point of checking if a value is null, because in the new version there is new |
I'm gonna try to explain myself with examples. Let's say, like in your documentation, that yout have an object in your Class, called 'adress' which has two property inside: 'city' and 'zipcode'. And on this object, you have you're
And also, that those two (city and zipcode) aren't required, except if one or the other has been filled. To be clear, if i don't fill either city or zipcode, it's fine. But if i fill for example city, then i have to also fill zipcode. Which would means to have a
The idea here would be to check with the I've even actually already built my own validators for what i need (thanks to your awesome
He's basically checking if the fieldValue is an object (like adress on your example), and if not, take the entire class to get all property inside it.Then it check if the property exist and if yes, if the property has is value filled. And i'm using it like this inside the
I hope it's clear enough... |
Ok, now I understand what you mean. Your case is very specific and I don't think that such validator would fit other developers' needs. But that's exactly what for are custom validators :) as you said. Your validation rule is rather like Maybe it's good idea to group validators. I will add this feature for consideration. I would have to think how it could be implemented to not mess with the current code. For now, I advice you to use your code. It does its job. I don't want to introduce such validators because I decided that validators should work on single field, eventually it should be based on the value of an another field but not existence of this field or its validation. |
I've actually came to the same conclusion while reading my own post ;). On a side note, would be nice with a "group validator" to be able to actually trigger the error message on a different field than the one in validation. For example, still with the adress example, easily being able to trigger an "Incomplete adress" on the |
In my list of features to implement I have the feature that would allow settting custom errors. Right now indeed it would be very hard to implement. You would need to mess with Astronomy internals. I'm planning to create some utils functions that would make creating custom validators even easier. The question is how group validators should work. Should they be treated as a new field? So, it would required to name a group. So you would be able to get validation error for a group. Should there be only groups of type "All fields in a group have to be valid" or maybe also "If any of the fields in a group is valid". Those groups would use |
Ok, nice to hear for the custom errors system ;). About the group validators. I don't think it should be seen as a new field. Seeing how powerful with just a few lines a Validators can already get, i'm not sure anything else than adding a new Validators similar to Something like |
Or, if i follow you're idea correctly, you'd have something like this ?
|
By saying "treated as a field" I didn't mean that it should be added as a field. And I didn't want it to be another validator but rather new feature. I will give you an example. Post = Astro.Class({
name: 'Post',
fields: {
first: {
type: 'string',
validators: Validators.length(5)
},
second: {
type: 'string',
validators: Validators.length(5)
},
third: {
type: 'string',
validators: Validators.length(5)
}
},
validationGroup: {
address: {
type: 'every',
fields: ['first', 'second', 'third']
}
}
}); Validation groups would be taken into account when validating entire document or on multiple fields validation where all the fields from the group are being validated. post.getValidationError('address'); What do you think? Maybe it's not the best approach but it was my first thought. |
Ok, now i get it. And basically, with your We should also have a property inside a validationGroup to directly setup the error message if it fails. |
Yes right. We will come back to details when I start implementing it :-) |
I was looking at the next version of your has() validators and like to throw in some idea about it.
I was mislead at first because of the name, which doesn't point out that it's only working on nested objects...I think it should like you did test if the fieldValue is an object, but if not, it should take the entire Class properties, and test it against the 'propertyName'.
Also, the _.has() function only test if a key exist in an object, but if that property as a null value, it doesn't care. So maybe it should also verified that the property actually has some values with it ?
What do you think ?
ps: I'd be happy to fork and work on that if you think my idea is any good...
The text was updated successfully, but these errors were encountered: