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.

Sign up for GitHub

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

removed extra + sign from country codes #2084

Merged
merged 3 commits into from
Jul 20, 2020
Merged

removed extra + sign from country codes #2084

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
25d3443
removed extra + sign from country codes
sudeeptarlekar Jul 20, 2020
7ba6c6f
Added phone number class in yaml files
sudeeptarlekar Jul 20, 2020
0ec2ea9
Corrected spell
sudeeptarlekar Jul 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions lib/locales/en-AU.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,13 @@ en-AU:
phone_number:
area_code: ['02', '03', '07', '08']
subscriber_number: ['####']
formats: ["#{area_code} 7010 #{subscriber_number}", '7010 ####', "(#{area_code}) 7010 #{subscriber_number}", '5550 ####', "#{area_code} 5550 #{subscriber_number}", "(#{area_code}) 5550 #{subscriber_number}", "+#{country_code} #{area_code} 7010 #{subscriber_number}", "+#{country_code} #{area_code} 5550 #{subscriber_number}"]
formats:
- '7010 ####'
- '5550 ####'
- "#{area_code} 7010 #{subscriber_number}"
- "#{area_code} 5550 #{subscriber_number}"
- "(#{area_code}) 7010 #{subscriber_number}"
- "(#{area_code}) 5550 #{subscriber_number}"
cell_phone:
formats: ['0491 570 006', '0491 570 156', '(04) 91570157', '0491 570 158', '0491 570 159', '0491570110', '0491 57 03 13', '0491 570 737', '0491 571 266', '0491 57 14 91', '0491 571 804', '0491 572 549', '0491 5726 65', '0491572983', '0491 573 770', '0491 573 087', '0491 574 118', '0491574632', '(04) 9157 5254', '0491 575 789', '0491 576 398', '0491576801', '0491 577 426', '0491577644', '0491 578 957', '0491 578 148', '0491 578 888', '0491 5792 12', '0491 579 760', '(04) 9157 9455']
team:
Expand All @@ -59,4 +65,4 @@ en-AU:
university:
name: [University of South Australia, University of Tasmania, University of Technology Sydney, Australian National University, University of Melbourne, Monash University, University of Sydney, University of Queensland, The University of Western Australia, Curtin University, Murdoch University, University of Adelaide, Macquarie University, Deakin University, Griffith University, Flinders University, Edith Cowan University, Australia Catholic University, Charles Sturt University, University of New South Wales]
bank:
name: [ANZ, Westpac, Commonwealth Bank, National Australia Bank, St George Bank, Bank of Queensland, Bankwest, Bendigo Bank, Credit Union Australia, ING]
name: [ANZ, Westpac, Commonwealth Bank, National Australia Bank, St George Bank, Bank of Queensland, Bankwest, Bendigo Bank, Credit Union Australia, ING]
3 changes: 2 additions & 1 deletion lib/locales/en-IND.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,5 @@ en-IND:
company:
suffix: [Pvt Ltd, Limited, Ltd, and Sons, Corp, Group, Brothers]
phone_number:
formats: ['+91###-###-####', '+91##########', '+91-###-#######']
formats: ['###-###-####', '##########', '###-#######']
country_code: ['91']
16 changes: 8 additions & 8 deletions lib/locales/en-US.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ en-US:
WV: '247##'
WI: '549##'
WY: '831##'
country_code: ['+1']
country_code: ['1']
phone_number:
subscriber_number: ['####']
extension: ['####']
Expand All @@ -94,23 +94,23 @@ en-US:
formats:
- "#{PhoneNumber.area_code}-#{PhoneNumber.exchange_code}-#{PhoneNumber.subscriber_number}"
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@sudeeptarlekar sudeeptarlekar Jul 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 `+).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

- "(#{PhoneNumber.area_code}) #{PhoneNumber.exchange_code}-#{PhoneNumber.subscriber_number}"
- "1-#{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}"
- "#{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}"
Copy link
Member

Choose a reason for hiding this comment

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

Is removing the number 1 intended ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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. :-)

Copy link
Contributor Author

@sudeeptarlekar sudeeptarlekar Jul 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

- "#{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} x#{PhoneNumber.extension}"
- "(#{PhoneNumber.area_code}) #{PhoneNumber.exchange_code}-#{PhoneNumber.subscriber_number} x#{PhoneNumber.extension}"
- "1-#{PhoneNumber.area_code}-#{PhoneNumber.exchange_code}-#{PhoneNumber.subscriber_number} x#{PhoneNumber.extension}"
- "#{PhoneNumber.area_code}-#{PhoneNumber.exchange_code}-#{PhoneNumber.subscriber_number} x#{PhoneNumber.extension}"
- "#{PhoneNumber.area_code}.#{PhoneNumber.exchange_code}.#{PhoneNumber.subscriber_number} x#{PhoneNumber.extension}"
- "#{PhoneNumber.area_code}-#{PhoneNumber.exchange_code}-#{PhoneNumber.subscriber_number} x#{PhoneNumber.extension}"
- "(#{PhoneNumber.area_code}) #{PhoneNumber.exchange_code}-#{PhoneNumber.subscriber_number} x#{PhoneNumber.extension}"
- "1-#{PhoneNumber.area_code}-#{PhoneNumber.exchange_code}-#{PhoneNumber.subscriber_number} x#{PhoneNumber.extension}"
- "#{PhoneNumber.area_code}-#{PhoneNumber.exchange_code}-#{PhoneNumber.subscriber_number} x#{PhoneNumber.extension}"
- "#{PhoneNumber.area_code}.#{PhoneNumber.exchange_code}.#{PhoneNumber.subscriber_number} x#{PhoneNumber.extension}"
- "#{PhoneNumber.area_code}-#{PhoneNumber.exchange_code}-#{PhoneNumber.subscriber_number} x#{PhoneNumber.extension}"
- "(#{PhoneNumber.area_code}) #{PhoneNumber.exchange_code}-#{PhoneNumber.subscriber_number} x#{PhoneNumber.extension}"
- "1-#{PhoneNumber.area_code}-#{PhoneNumber.exchange_code}-#{PhoneNumber.subscriber_number} x#{PhoneNumber.extension}"
- "#{PhoneNumber.area_code}-#{PhoneNumber.exchange_code}-#{PhoneNumber.subscriber_number} x#{PhoneNumber.extension}"
- "#{PhoneNumber.area_code}.#{PhoneNumber.exchange_code}.#{PhoneNumber.subscriber_number} x#{PhoneNumber.extension}"

id_number:
Expand All @@ -121,9 +121,9 @@ en-US:
formats:
- "#{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}"
- "#{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}"
- "(#{PhoneNumber.area_code}) #{PhoneNumber.exchange_code}-#{PhoneNumber.subscriber_number}"
- "1-#{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}"
24 changes: 19 additions & 5 deletions lib/locales/fr-CA.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,24 +26,38 @@ fr-CA:
formats:
- "#{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}"
- "#{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}"
- "(#{PhoneNumber.area_code}) #{PhoneNumber.exchange_code}-#{PhoneNumber.subscriber_number}"
- "1-#{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}"
- "#{PhoneNumber.area_code}-#{PhoneNumber.exchange_code}-#{PhoneNumber.subscriber_number} x#{PhoneNumber.extension}"
- "(#{PhoneNumber.area_code}) #{PhoneNumber.exchange_code}-#{PhoneNumber.subscriber_number} x#{PhoneNumber.extension}"
- "1-#{PhoneNumber.area_code}-#{PhoneNumber.exchange_code}-#{PhoneNumber.subscriber_number} x#{PhoneNumber.extension}"
- "#{PhoneNumber.area_code}-#{PhoneNumber.exchange_code}-#{PhoneNumber.subscriber_number} x#{PhoneNumber.extension}"
- "#{PhoneNumber.area_code}.#{PhoneNumber.exchange_code}.#{PhoneNumber.subscriber_number} x#{PhoneNumber.extension}"
- "#{PhoneNumber.area_code}-#{PhoneNumber.exchange_code}-#{PhoneNumber.subscriber_number} x#{PhoneNumber.extension}"
- "(#{PhoneNumber.area_code}) #{PhoneNumber.exchange_code}-#{PhoneNumber.subscriber_number} x#{PhoneNumber.extension}"
- "1-#{PhoneNumber.area_code}-#{PhoneNumber.exchange_code}-#{PhoneNumber.subscriber_number} x#{PhoneNumber.extension}"
- "#{PhoneNumber.area_code}-#{PhoneNumber.exchange_code}-#{PhoneNumber.subscriber_number} x#{PhoneNumber.extension}"
- "#{PhoneNumber.area_code}.#{PhoneNumber.exchange_code}.#{PhoneNumber.subscriber_number} x#{PhoneNumber.extension}"
- "#{PhoneNumber.area_code}-#{PhoneNumber.exchange_code}-#{PhoneNumber.subscriber_number} x#{PhoneNumber.extension}"
- "(#{PhoneNumber.area_code}) #{PhoneNumber.exchange_code}-#{PhoneNumber.subscriber_number} x#{PhoneNumber.extension}"
- "1-#{PhoneNumber.area_code}-#{PhoneNumber.exchange_code}-#{PhoneNumber.subscriber_number} x#{PhoneNumber.extension}"
- "#{PhoneNumber.area_code}-#{PhoneNumber.exchange_code}-#{PhoneNumber.subscriber_number} x#{PhoneNumber.extension}"
- "#{PhoneNumber.area_code}.#{PhoneNumber.exchange_code}.#{PhoneNumber.subscriber_number} x#{PhoneNumber.extension}"
cell_phone:
formats:
- "#{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}"
- "#{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}"
- "#{PhoneNumber.area_code}-#{PhoneNumber.exchange_code}-#{PhoneNumber.subscriber_number}"
- "#{PhoneNumber.area_code}.#{PhoneNumber.exchange_code}.#{PhoneNumber.subscriber_number}"

country_code:
- '1'

company:
suffix: [SARL, SA, EURL, SAS, SEM, SCOP, GIE, EI]
# Buzzword wordlist from http://www.1728.com/buzzword.htm
Expand Down