-
-
Notifications
You must be signed in to change notification settings - Fork 106
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
Add more string validators #15
Conversation
source/lib/predicates/string.ts
Outdated
/** | ||
* Test a string to be email like. | ||
*/ | ||
get email() { |
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 don't think we should do this one. I can't think of when I would use it except for forms and then there's type="email"
. There are so many obscure cases to support with this and I don't think we'll want to deal with all the issue/support overhead.
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.
Yes you're right, if they really want it, they will be able to use the generic is()
method to pass in a function.
source/lib/predicates/string.ts
Outdated
*/ | ||
get numeric() { | ||
return this.addValidator({ | ||
message: value => `Expected string to contain only numeric characters but received \`${value}\``, |
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.
Should the message deal with what it received? Couldn't we generalize and automate that for all predicates? At least some kind of helper.
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 you mean some auto-formatted message?
Expected to be , got
${value}
Resulting in this case
Expected string to be numeric, got
foo1
Registering a validator could then look like
return this.addValidator({
name: 'numeric',
validator: value => /^[\d]+$/i.test(value)
});
Whereas the provided name will be inserted in the predefined error message. This would also fix our issue regarding the messages with the not
predicate #12 (comment). We can easily invert the statement
Expected to not be , got
${value}
Which results in
Expected string to not be numeric, got
123
Is this what you mean? :p
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.
Yes, ideally, but not sure whether it would work in all cases. Maybe we should just use the full sentences for now and then see how we can generalize it later when we have all the validators we want?
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.
Good idea. I will make sure that the error messages are all structured the same way so we can easily switch to a more generic version in the future.
@@ -1,3 +1,4 @@ | |||
import * as valiDate from 'vali-date'; |
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.
Let me know if you want to remove this dependency together with the date
validator. It can always be covered later with the .is(valiDate)
.
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.
No, it's fine. Keep it.
source/lib/predicates/string.ts
Outdated
get numeric() { | ||
return this.addValidator({ | ||
message: value => `Expected string to be numeric, got \`${value}\``, | ||
validator: value => /^[\d]+$/i.test(value) |
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.
[\d]
=> \d
This looks good to me. Awesome to have so many validators. |
Sweet! :) |
Just the start of completing the string validators. Tests are coming up. But this way, the things I already implemented can be verified (docs, ...).