Skip to content

- phone numbers in Canada are of the same format as those in the USA.…#146

Open
jazty wants to merge 4 commits into
cakephp:4.xfrom
jazty:master
Open

- phone numbers in Canada are of the same format as those in the USA.…#146
jazty wants to merge 4 commits into
cakephp:4.xfrom
jazty:master

Conversation

@jazty

@jazty jazty commented Jan 25, 2017

Copy link
Copy Markdown

… No need to throw NotImplemented. Copied the US phone validation rules to Canada.

… No need to throw NotImplemented. Copied the US phone validation rules to Canada.
@dereuromark

Copy link
Copy Markdown
Member

Can we have some tests for this please?

@jazty

jazty commented Jan 25, 2017

Copy link
Copy Markdown
Author

Copied the tests as well and they all pass.

@dereuromark

Copy link
Copy Markdown
Member

Thx, looks good to me 👍

Jesse Aleksanter Kahtava added 2 commits January 26, 2017 13:10
}
}

class InvalidNumberRule

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Member

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.

@jazty jazty Jan 26, 2017

Copy link
Copy Markdown
Author

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.

Copy link
Copy Markdown
Member

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.

@dereuromark

Copy link
Copy Markdown
Member

@jazty Still interested in getting this finished up and merged?

@othercorey

Copy link
Copy Markdown
Contributor

Should we trim and merge or close this?

@othercorey othercorey changed the base branch from master to 4.x October 16, 2021 07:03
@dereuromark

Copy link
Copy Markdown
Member

If we can trim and merge, that would probably be best.
Just closing is easiest, but then nothing is gained.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants