-
Notifications
You must be signed in to change notification settings - Fork 25.1k
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
feat(forms): Validator.pattern accepts a RegExp #12323
Conversation
91a460c
to
a459f16
Compare
@@ -7,6 +7,7 @@ | |||
*/ | |||
|
|||
import {OpaqueToken} from '@angular/core'; | |||
import {isString} from '@angular/facade/src/lang'; |
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.
Please don't use facades, we are in the process of removing them
return (control: AbstractControl): {[key: string]: any} => { | ||
if (isEmptyInputValue(control.value)) { | ||
return null; // don't validate empty values to allow optional controls | ||
} | ||
const regex = new RegExp(`^${pattern}$`); | ||
const regex: RegExp = isString(pattern) ? new RegExp(`^${pattern}$`) : <RegExp>pattern; |
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 the type necessary in const regex: RegExp
?
02cd004
to
eb04c19
Compare
return (control: AbstractControl): {[key: string]: any} => { | ||
if (isEmptyInputValue(control.value)) { | ||
return null; // don't validate empty values to allow optional controls | ||
} | ||
const regex = new RegExp(`^${pattern}$`); | ||
const regex = typeof pattern === 'string' ? new RegExp(`^${pattern}$`) : <RegExp>pattern; |
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.
<RegExp>
should not be needed
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.
tsc complains (at least in ide)
const value: string = control.value; | ||
return regex.test(value) ? | ||
return regex && regex.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.
not sure of the !regex
case ?
Should it rather returns an empty ValidatorFn
? (_) => null
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.
Add a test
f7b92de
to
f8ea777
Compare
|
||
it('should error on failure to match string', () => { | ||
expect(Validators.pattern('[a-zA-Z ]*')(new FormControl('aaa0'))).toEqual({ | ||
'pattern': {'requiredPattern': '^[a-zA-Z ]*$', 'actualValue': 'aaa0'} | ||
'pattern': {'requiredPattern': '/^[a-zA-Z ]*$/', 'actualValue': 'aaa0'} |
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.
Could you please remove this breaking change ? (we would have to delay the PR until 3.0). Thanks
f26e4ab
to
58231d4
Compare
const value: string = control.value; | ||
return regex.test(value) ? | ||
null : | ||
{'pattern': {'requiredPattern': `^${pattern}$`, 'actualValue': value}}; | ||
{'pattern': {'requiredPattern': `${regexStr}`, 'actualValue': 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.
Why not just directly put regexStr
?
58231d4
to
b893bb8
Compare
b893bb8
to
6206799
Compare
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Fixes #10150