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(location): no leading zero on building number or secondary address #2032

Merged

Conversation

matthewmayer
Copy link
Contributor

@matthewmayer matthewmayer commented Apr 9, 2023

fix #2031

Note this only affects default en locale. Similar approach could be taken in other locales which have building_number patterns

@matthewmayer matthewmayer requested a review from a team as a code owner April 9, 2023 14:11
@matthewmayer matthewmayer self-assigned this Apr 9, 2023
@matthewmayer matthewmayer added p: 1-normal Nothing urgent c: locale Permutes locale definitions m: location Something is referring to the location module labels Apr 9, 2023
@codecov
Copy link

codecov bot commented Apr 9, 2023

Codecov Report

Merging #2032 (d6ad01b) into next (a94d365) will increase coverage by 0.00%.
The diff coverage is 88.88%.

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #2032   +/-   ##
=======================================
  Coverage   99.59%   99.59%           
=======================================
  Files        2567     2567           
  Lines      243388   243394    +6     
  Branches     1251     1253    +2     
=======================================
+ Hits       242404   242417   +13     
+ Misses        957      950    -7     
  Partials       27       27           
Impacted Files Coverage Δ
src/definitions/location.ts 0.00% <0.00%> (ø)
src/modules/location/index.ts 99.02% <100.00%> (+<0.01%) ⬆️

... and 1 file with indirect coverage changes

@ST-DDT
Copy link
Member

ST-DDT commented Apr 9, 2023

IMO instead of doing this strange pattern duplication stuff we should rather change the implementation to not use replacewithnumber and instead use a simple regex replace with numeric without leading zero or something.

@ST-DDT ST-DDT added the s: needs decision Needs team/maintainer decision label Apr 9, 2023
@ST-DDT
Copy link
Member

ST-DDT commented Apr 20, 2023

Team Decision

  • We will implement this via arrayElement(patterns).replace(/#+/g, (m) => string.numeric(m.length)
  • The locale definition documentation will be extended to explicitly mention the # -> number transformation
  • We will add a test that checks that it will not return leading zeros

See #2031 (comment)

@matthewmayer Will you update your PR or start a new one?

@ST-DDT ST-DDT added do NOT merge yet Do not merge this PR into the target branch yet and removed s: needs decision Needs team/maintainer decision labels Apr 20, 2023
@matthewmayer matthewmayer removed the do NOT merge yet Do not merge this PR into the target branch yet label Apr 30, 2023
@matthewmayer
Copy link
Contributor Author

Would it also make sense to apply this for secondary address? To avoid "Apt 000" in en or '3/0' in pl etc.

@matthewmayer
Copy link
Contributor Author

Would it also make sense to apply this for secondary address? To avoid "Apt 000" in en or '3/0' in pl etc.

in this PR or a seperate PR?

ST-DDT
ST-DDT previously approved these changes Apr 30, 2023
@ST-DDT
Copy link
Member

ST-DDT commented Apr 30, 2023

in this PR or a seperate PR?

Since it does the exact same thing for related stuff, I am fine with doing it in one PR.
Not sure what the others think though.

The PR title needs to be adjusted anyway.

@matthewmayer matthewmayer changed the title fix(location): no leading zero on building number in en fix(location): no leading zero on building number or secondary address Apr 30, 2023
@ST-DDT ST-DDT requested review from Shinigami92 and a team April 30, 2023 11:43
@ST-DDT ST-DDT added the s: accepted Accepted feature / Confirmed bug label Apr 30, 2023
@ST-DDT ST-DDT enabled auto-merge (squash) May 1, 2023 08:57
@ST-DDT ST-DDT merged commit a8dc7e0 into faker-js:next May 1, 2023
15 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: location Something is referring to the location module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Don't allow faker.location.buildingNumber() to start with a 0
4 participants