-
Notifications
You must be signed in to change notification settings - Fork 176
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
- phone numbers in Canada are of the same format as those in the USA.… #146
base: 4.x
Are you sure you want to change the base?
Conversation
… No need to throw NotImplemented. Copied the US phone validation rules to Canada.
Can we have some tests for this please? |
Copied the tests as well and they all pass. |
Thx, looks good to me 👍 |
lot of problems and was difficult to maintain.
@@ -59,3 +108,101 @@ public static function personId($check) | |||
throw new NotImplementedException(__d('localized', '%s Not implemented yet.')); | |||
} | |||
} | |||
|
|||
class InvalidNumberRule |
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 did you implement this now? The previous PR was good to go, but now you introduce a lot of debatable code pieces here.
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 that a separate class over regexp wouldn't be a viable option, but this looks like a snowflake between all the other validation rules if not applied on a more discussed and global level.
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 pushed it to my own fork. Didn't think it would trickle down... I just needed something more robust and the regex was not working for some real area codes (likewise, it let some invalid area codes through). You can ignore it... or is there something I can do to hide it? I'll look into that.
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.
You can rebase, sqash and we can sure merge it then for now :) Thx.
@jazty Still interested in getting this finished up and merged? |
Should we trim and merge or close this? |
If we can trim and merge, that would probably be best. |
… No need to throw NotImplemented. Copied the US phone validation rules to Canada.