-
Notifications
You must be signed in to change notification settings - Fork 175
Add must_not_match validator #365
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
Conversation
|
@Keats ping :) |
validator_derive/src/lib.rs
Outdated
|
|
||
| // Must not match validation | ||
| let must_not_match = if let Some(must_not_match) = self.must_not_match.clone() { | ||
| // TODO: handle option for other |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that still a TODO?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understood correctly - it was handled
| /// Validates that the 2 given fields do not match. | ||
| /// Both fields are optionals | ||
| #[must_use] | ||
| pub fn validate_must_not_match<T: Eq>(a: T, b: T) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want Eq or PartialEq?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, should be PartialEq
1dc4cb8 to
4d3607a
Compare
|
Looks like legit failures |
I fixed tests |
|
@Keats reminder :) |
1 similar comment
|
@Keats reminder :) |
|
|
||
| ### must_match | ||
| Tests whether the 2 fields are equal. `must_match` takes 1 string argument. It will error if the field | ||
| ### must_match and must_not_match |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we can use must_match rather than create a new validator. Something like must_match(expected=false) with a default of true (bad name but just to illustrate)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you didn't approve generic not() (Add not_match validator #141) and we have contains and does_not_contain I thought that we need a new one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, can we move on?
|
After thinking about it in practice it would be easier to write a custom validator and doesn't make sense to be a default validator. I probably wouldn't even include |
|
@Keats no problem |
|
You can't right now, you have to do a schema level validation fn |
Hi there! I want to add one more validator if you don't mind