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(locale): improve Swedish phone numbers format #2520

Merged
merged 7 commits into from
Nov 7, 2023

Conversation

Gyran
Copy link
Contributor

@Gyran Gyran commented Oct 31, 2023

https://en.wikipedia.org/wiki/Telephone_numbers_in_Sweden

Numbers starting with 070, 072, 073, 076 or 079 are reserved for mobile numbers in Sweden.

https://en.wikipedia.org/wiki/Telephone_numbers_in_Sweden

Numbers starting with 070, 072, 073, 076 or 079 are reserved for mobile numbers in Sweden.
@Gyran Gyran requested a review from a team as a code owner October 31, 2023 23:43
Copy link

codecov bot commented Oct 31, 2023

Codecov Report

Merging #2520 (e7fd39e) into next (e0ba50b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #2520   +/-   ##
=======================================
  Coverage   99.57%   99.58%           
=======================================
  Files        2779     2779           
  Lines      249273   249303   +30     
  Branches     1080     1083    +3     
=======================================
+ Hits       248217   248265   +48     
+ Misses       1028     1010   -18     
  Partials       28       28           
Files Coverage Δ
src/locales/sv/phone_number/formats.ts 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

@matthewmayer
Copy link
Contributor

Note that faker.phone.number() does not specifically promise to return mobile numbers. Can also be landlines. If you have a use-case for mobile numbers only it might be good to get your input on #1542

@Gyran
Copy link
Contributor Author

Gyran commented Nov 2, 2023

That's true, I could add formats for landline numbers as well. But the current formats does not look like any Swedish numbers. So I thought that this is at least make the SV faker version return Swedish phone number (but not all kinds).
For my use case I only want mobile numbers and I think that is true for most Swedish use cases. But it would be nice as you say to be able to specify that.

Will write in the other issue as well, but in the meantime, is this change something you would be interested in or how can I change it to follow the guidelines? Add these formats to the cell formats instead?

@matthewmayer
Copy link
Contributor

If it's an improvement from the current data we should accept this PR anyway.

At the moment the cell_phone definitions are not actively used by Faker.

You can have a read through the discussion on the other issue and add any comments on how you would typically use this data to help guide us on what would be most useful in future.

matthewmayer
matthewmayer previously approved these changes Nov 2, 2023
@ST-DDT
Copy link
Member

ST-DDT commented Nov 2, 2023

I could add formats for landline numbers as well.

I think it would be best to include them for now so that we have them already when we split/improve the method

@ST-DDT ST-DDT added p: 1-normal Nothing urgent c: locale Permutes locale definitions m: phone Something is referring to the phone module labels Nov 2, 2023
@ST-DDT ST-DDT added this to the vAnytime milestone Nov 2, 2023
@ST-DDT
Copy link
Member

ST-DDT commented Nov 2, 2023

And please also include ones with the international prefix for variation.

@ST-DDT ST-DDT requested review from matthewmayer and a team November 6, 2023 21:00
@ST-DDT ST-DDT changed the title fix(locale): Use Swedish mobile phone numbers format fix(locale): improve Swedish phone numbers format Nov 7, 2023
@ST-DDT ST-DDT enabled auto-merge (squash) November 7, 2023 20:46
@ST-DDT ST-DDT merged commit e4865df into faker-js:next Nov 7, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: locale Permutes locale definitions m: phone Something is referring to the phone module p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants