removed extra + sign from country codes #2084

sudeeptarlekar · 2020-07-20T08:37:26Z

resolves: #2060

Description:

Removed extra + sign from the cell phone in e164 format for en-US locale and refactored the code

lib/faker/version.rb

psibi · 2020-07-20T09:17:57Z

lib/locales/en-US.yml

    phone_number:
      subscriber_number: ['####']
      extension: ['####']
      area_code: ["201", "202", "203", "205", "206", "207", "208", "209", "210", "212", "213", "214", "215", "216", "217", "218", "219", "224", "225", "226", "228", "229", "231", "234", "239", "240", "248", "251", "252", "253", "254", "256", "260", "262", "267", "269", "270", "276", "281", "301", "302", "303", "304", "305", "307", "308", "309", "310", "312", "313", "314", "315", "316", "317", "318", "319", "320", "321", "323", "330", "334", "336", "337", "339", "347", "351", "352", "360", "361", "386", "401", "402", "404", "405", "406", "407", "408", "409", "410", "412", "413", "414", "415", "417", "419", "423", "424", "425", "434", "435", "440", "443", "469", "478", "479", "480", "484", "501", "502", "503", "504", "505", "507", "508", "509", "510", "512", "513", "515", "516", "517", "518", "520", "530", "540", "541", "551", "559", "561", "562", "563", "567", "570", "571", "573", "574", "580", "585", "586", "601", "602", "603", "605", "606", "607", "608", "609", "610", "612", "614", "615", "616", "617", "618", "619", "620", "623", "626", "630", "631", "636", "641", "646", "650", "651", "660", "661", "662", "678", "682", "701", "702", "703", "704", "706", "707", "708", "712", "713", "714", "715", "716", "717", "718", "719", "720", "724", "727", "731", "732", "734", "740", "754", "757", "760", "763", "765", "770", "772", "773", "774", "775", "781", "785", "786", "801", "802", "803", "804", "805", "806", "808", "810", "812", "813", "814", "815", "816", "817", "818", "828", "830", "831", "832", "843", "845", "847", "848", "850", "856", "857", "858", "859", "860", "862", "863", "864", "865", "870", "878", "901", "903", "904", "906", "907", "908", "909", "910", "912", "913", "914", "915", "916", "917", "918", "919", "920", "925", "928", "931", "936", "937", "940", "941", "947", "949", "952", "954", "956", "970", "971", "972", "973", "978", "979", "980", "985", "989"]
      exchange_code: ["201", "202", "203", "205", "206", "207", "208", "209", "210", "212", "213", "214", "215", "216", "217", "218", "219", "224", "225", "227", "228", "229", "231", "234", "239", "240", "248", "251", "252", "253", "254", "256", "260", "262", "267", "269", "270", "276", "281", "283", "301", "302", "303", "304", "305", "307", "308", "309", "310", "312", "313", "314", "315", "316", "317", "318", "319", "320", "321", "323", "330", "331", "334", "336", "337", "339", "347", "351", "352", "360", "361", "386", "401", "402", "404", "405", "406", "407", "408", "409", "410", "412", "413", "414", "415", "417", "419", "423", "424", "425", "434", "435", "440", "443", "445", "464", "469", "470", "475", "478", "479", "480", "484", "501", "502", "503", "504", "505", "507", "508", "509", "510", "512", "513", "515", "516", "517", "518", "520", "530", "540", "541", "551", "557", "559", "561", "562", "563", "564", "567", "570", "571", "573", "574", "580", "585", "586", "601", "602", "603", "605", "606", "607", "608", "609", "610", "612", "614", "615", "616", "617", "618", "619", "620", "623", "626", "630", "631", "636", "641", "646", "650", "651", "660", "661", "662", "667", "678", "682", "701", "702", "703", "704", "706", "707", "708", "712", "713", "714", "715", "716", "717", "718", "719", "720", "724", "727", "731", "732", "734", "737", "740", "754", "757", "760", "763", "765", "770", "772", "773", "774", "775", "781", "785", "786", "801", "802", "803", "804", "805", "806", "808", "810", "812", "813", "814", "815", "816", "817", "818", "828", "830", "831", "832", "835", "843", "845", "847", "848", "850", "856", "857", "858", "859", "860", "862", "863", "864", "865", "870", "872", "878", "901", "903", "904", "906", "907", "908", "909", "910", "912", "913", "914", "915", "916", "917", "918", "919", "920", "925", "928", "931", "936", "937", "940", "941", "947", "949", "952", "954", "956", "959", "970", "971", "972", "973", "975", "978", "979", "980", "984", "985", "989"]
      formats:
-        - "#{PhoneNumber.area_code}-#{PhoneNumber.exchange_code}-#{PhoneNumber.subscriber_number}"


Any specific reason on why the PhoneNumber prefix is being removed ?

I found PhoneNumber in YAML has no use. I can add it back if you point me where we are using area_code, extension_code other than phone numbers or is there any particular reason for adding PhoneNumber class? Ref

The downstream consumers of Ruby's locale depends on it. Eg: https://github.com/psibi/fakedata/blob/d08b7b9ff41f3f91e3376ec573cf58f7a95c30b2/src/Faker/Provider/PhoneNumber.hs#L119

Please help me understand here, if you change locale to en-AU. is that suppose to work? As for AU locale we are not using class name in YML.

I would like to hear what others think about it or else will revert the change.

As for AU locale

Note that it's not dependent on AU locale in the downstream client. I do like this cleanup. I'm just confused if removing this was needed for your fix (removing the `+).

No it was not needed, I just cleaned up code. But we can take that later. Will create a issue for same. Will revert change.

Thanks for review @psibi

I would leave it your choice. But I don't think reverting is needed. You could just probably update the MR description.

psibi · 2020-07-20T09:27:17Z

lib/locales/en-US.yml

-        - "#{PhoneNumber.area_code}.#{PhoneNumber.exchange_code}.#{PhoneNumber.subscriber_number}"
-        - "#{PhoneNumber.area_code}-#{PhoneNumber.exchange_code}-#{PhoneNumber.subscriber_number}"
-        - "(#{PhoneNumber.area_code}) #{PhoneNumber.exchange_code}-#{PhoneNumber.subscriber_number}"
-        - "1-#{PhoneNumber.area_code}-#{PhoneNumber.exchange_code}-#{PhoneNumber.subscriber_number}"


Is removing the number 1 intended ?

1 here again is not being used while parsing, it is getting omitted after scanning in parse method. I see I missed a - there

If the idea is to replace this with the country code, probably the 1 should be replaced with #{PhoneNumber.country_code} instead ?

Although for countries, with only one country_code - I'm not sure if we gain anything by introducing a new field and referring it via #{.. syntax. (We will lose somewhat in perf because of extra de-referencing) Probably, it's best to have it as 1. I'm okay with the additional field though. :-)

Problem of introducing country code in format is if we call method phone_number_with_country_code then it may return phone number as +1 +1-###-###-#### which is also wrong. Many locale are using country code in format, but thats for another PR.

Thoughts?

Ah, I see what you mean. I didn't know about the existence of that method. In that case, I think only removing the prefix - should do.

psibi · 2020-07-20T12:22:58Z

Thanks @sudeeptarlekar !

Fix extra + sign in phone numbers

removed extra + sign from country codes

25d3443

koic reviewed Jul 20, 2020

View reviewed changes

lib/faker/version.rb Outdated Show resolved Hide resolved

psibi reviewed Jul 20, 2020

View reviewed changes

sudeeptarlekar force-pushed the 2060-wrong-phone-number-for-en-US branch from 5d1af74 to 25d3443 Compare July 20, 2020 10:15

sudeeptarlekar added 2 commits July 20, 2020 16:58

Added phone number class in yaml files

7ba6c6f

Corrected spell

0ec2ea9

sudeeptarlekar requested a review from psibi July 20, 2020 12:05

psibi approved these changes Jul 20, 2020

View reviewed changes

psibi merged commit d7de988 into faker-ruby:master Jul 20, 2020

sudeeptarlekar deleted the 2060-wrong-phone-number-for-en-US branch July 21, 2020 04:37

gabrielbaldao pushed a commit to gabrielbaldao/faker that referenced this pull request Jul 22, 2020

removed extra + sign from country codes (faker-ruby#2084)

c929e6d

Fix extra + sign in phone numbers

sudeeptarlekar mentioned this pull request Sep 25, 2020

US phone numbers have an extra + #2125

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

removed extra + sign from country codes #2084

removed extra + sign from country codes #2084

sudeeptarlekar commented Jul 20, 2020

psibi Jul 20, 2020

sudeeptarlekar Jul 20, 2020 •

edited

Loading

psibi Jul 20, 2020

sudeeptarlekar Jul 20, 2020

psibi Jul 20, 2020

sudeeptarlekar Jul 20, 2020

psibi Jul 20, 2020

psibi Jul 20, 2020

sudeeptarlekar Jul 20, 2020

psibi Jul 20, 2020

psibi Jul 20, 2020

sudeeptarlekar Jul 20, 2020 •

edited

Loading

psibi Jul 20, 2020

psibi commented Jul 20, 2020

removed extra + sign from country codes #2084

removed extra + sign from country codes #2084

Conversation

sudeeptarlekar commented Jul 20, 2020

Description:

Choose a reason for hiding this comment

sudeeptarlekar Jul 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sudeeptarlekar Jul 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psibi commented Jul 20, 2020

sudeeptarlekar Jul 20, 2020 •

edited

Loading

sudeeptarlekar Jul 20, 2020 •

edited

Loading