Skip to content
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 Number Validation #277

Merged
merged 1 commit into from
Jun 6, 2022
Merged

Phone Number Validation #277

merged 1 commit into from
Jun 6, 2022

Conversation

uurl
Copy link
Contributor

@uurl uurl commented Jun 5, 2022

This PR solves #249, solves #250 and solves #251 with all the countries in the world

@uurl uurl requested a review from a team as a code owner June 5, 2022 09:29
}

/** Phone number validation for Gibraltar */
lazy val phoneNumberGI: Validation[String] = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that this is a very common pattern, probably would worth moving into a method with parameters for the country code and maybe the min/max number of digits to reduce code duplication.

(or maybe a couple of such methods if there are other common cases shared by many countries)

The more unique cases could remain as they are, of course.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, the point here is that each specific country has its own dialing rules and its own prefixes, in fact, the original issue only asked for US, UK and Europe. For example, as it stands, we can add whatever rules we want to the US and UK individually, but I don't really know what they want the scope of this validation to be. I'm going to refactor the code, but adding new specific validations for each country would lead to the same problem.

@jdegoes
Copy link
Member

jdegoes commented Jun 6, 2022

What a massive undertaking! Would be nice at some point to factor out some common code, but I am so impressed by the scope of this PR and grateful for this contribution, I am happy to merge as-is. Thank you so much & congratulations on your PR!

@jdegoes jdegoes merged commit 1737683 into zio:main Jun 6, 2022
@subotic
Copy link

subotic commented Jun 10, 2022

@uurl please don't just take other people's code without proper attribution. Not a nice thing to do.

@uurl uurl deleted the issue251 branch June 10, 2022 16:59
@jdegoes
Copy link
Member

jdegoes commented Jun 10, 2022

@uurl Proper attribution is extremely important in the ZIO community, and should either happen by officially basing your work off another person's pull request, so that they will receive "Github credit" for it, or in cases where this is not possible, crediting the developer with a note in the source code.

Thanks for your work building on @subotic's pull request, and to ensure he receives proper credit, please submit a followup pull request that, in a comment, notes the lines of code that should be credited.

Something like:

// Thanks to @subotic for his work developing this code

It might also be possible to git rewrite history, but I can't help you with that.

@subotic Thanks for writing code that formed part of this pull request, and we'll make sure you receive full attribution for it!

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.

4 participants