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

Fix code example in README to match description #167

Merged
merged 3 commits into from
Jun 20, 2017
Merged

Fix code example in README to match description #167

merged 3 commits into from
Jun 20, 2017

Conversation

mattruzicka
Copy link
Contributor

@mattruzicka mattruzicka commented Jun 20, 2017

The description for the code example says:

You can validate against the normalized input as opposed to the raw input:

but the code example is still validating the raw input. This example "works" as long as the user enters a US number since the validation defaults to "US", but breaks if a an "IT" number is entered, for example.

Also, since we're validating the already normalized phone number, I don't think providing the "normalized_country_code" option is useful.

EDIT:
I added commit e44e76e because I'm guessing in the most common use case people don't want their records to be valid if the user enters a random string like "whatever", but also don't want to force a phone number - if they do, they should validate that the "phone_number" is present.

Having the normalized_country_code option makes the code example confusing.
@coveralls
Copy link

coveralls commented Jun 20, 2017

Coverage Status

Coverage remained the same at 99.809% when pulling ff0a589 on mattruzicka:master into 9e268a7 on joost:master.

Validate normalized number is present since the phony_normalize method will normalize strings with random letters to nil. But only validate if phone_number is present.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.809% when pulling e44e76e on mattruzicka:master into 9e268a7 on joost:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.809% when pulling e44e76e on mattruzicka:master into 9e268a7 on joost:master.

@joost joost merged commit 9b002ec into joost:master Jun 20, 2017
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