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): Pad en_US ZIP codes left to 5 characters if needed #2278

Merged
merged 7 commits into from Aug 10, 2023

Conversation

matthewmayer
Copy link
Contributor

@matthewmayer matthewmayer commented Jul 26, 2023

fix #2277

Added a test and a fix. Not sure if this is the best long-term solution, but since only en_US uses this method currently, it seems a fairly low-impact solution.

Also fixed Puerto Rico range so its not always 00000

@matthewmayer matthewmayer requested a review from a team as a code owner July 26, 2023 20:56
@matthewmayer matthewmayer self-assigned this Jul 26, 2023
@matthewmayer matthewmayer added c: bug Something isn't working p: 1-normal Nothing urgent m: location Something is referring to the location module labels Jul 26, 2023
@codecov
Copy link

codecov bot commented Jul 27, 2023

Codecov Report

Merging #2278 (fbf3ca5) into next (7f0daf3) will decrease coverage by 0.01%.
The diff coverage is 94.91%.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #2278      +/-   ##
==========================================
- Coverage   99.60%   99.59%   -0.01%     
==========================================
  Files        2644     2644              
  Lines      245770   245617     -153     
  Branches     1085     1079       -6     
==========================================
- Hits       244798   244630     -168     
- Misses        945      960      +15     
  Partials       27       27              
Files Changed Coverage Δ
src/definitions/location.ts 0.00% <0.00%> (ø)
src/locales/en_US/location/postcode_by_state.ts 100.00% <100.00%> (ø)
src/modules/location/index.ts 99.17% <100.00%> (+0.16%) ⬆️

... and 1 file with indirect coverage changes

@ST-DDT
Copy link
Member

ST-DDT commented Jul 27, 2023

Team decision

This looks like a temporary workaround that is tailored for en-us only, but doesn't consider other zip code formats such as Poland's:
https://en.wikipedia.org/wiki/Postal_codes_in_Poland#Postal_codes_used_for_more_than_one_settlement
Maybe we should consider adding support for fake patterns instead.

@ahaberbosch
Copy link

It would be nice if ranges in addition to patterns could be specified. Realistically, most states don't seem to utilize the full range.
For example, AZ has a gap between 85087 and 85117, and several other gaps.

@ahaberbosch
Copy link

Team decision

This looks like a temporary workaround that is tailored for en-us only, but doesn't consider other zip code formats such as Poland's: https://en.wikipedia.org/wiki/Postal_codes_in_Poland#Postal_codes_used_for_more_than_one_settlement Maybe we should consider adding support for fake patterns instead.

There's already support for patterns, but it seems to be exclusive to the usage of zip code by state. Could they be merged?

@matthewmayer
Copy link
Contributor Author

Team decision

This looks like a temporary workaround that is tailored for en-us only, but doesn't consider other zip code formats such as Poland's: https://en.wikipedia.org/wiki/Postal_codes_in_Poland#Postal_codes_used_for_more_than_one_settlement Maybe we should consider adding support for fake patterns instead.

in many countries, postcodes/ZIP codes don't match up with state boundaries.

For example note in that Wikipedia article "Postal districts and their subdivisions are not related to the administrative division of Poland."

So I doubt there will be much usage of this outside of en_US.

@ahaberbosch
Copy link

Team decision
This looks like a temporary workaround that is tailored for en-us only, but doesn't consider other zip code formats such as Poland's: https://en.wikipedia.org/wiki/Postal_codes_in_Poland#Postal_codes_used_for_more_than_one_settlement Maybe we should consider adding support for fake patterns instead.

in many countries, postcodes/ZIP codes don't match up with state boundaries.

For example note in that Wikipedia article "Postal districts and their subdivisions are not related to the administrative division of Poland."

So I doubt there will be much usage of this outside of en_US.

Actually, it looks like the proposed PR could be useful for Mexico, specifically the mexico City boroughs. The Mexican zip codes are modelled after US zip codes, with some mexico city zip codes prefixed with zeroes.

@RobinvanderVliet
Copy link
Contributor

So I doubt there will be much usage of this outside of en_US.

I tried implementing it for the Netherlands and our provinces, but I ran into a problem as our postal codes also contain letters. Dutch postal codes are in the format /[1-9][0-9]{3} [A-Z]{2}/.

I found the numeric ranges for the provinces, and for the letters we could just use 2 random letters.

@matthewmayer
Copy link
Contributor Author

matthewmayer commented Jul 27, 2023

My feeling is while we should support this for legacy reasons for en_US we shouldn't put too much effort into trying to make this work in every locale- if this feature didn't currently exist I suspect it would be rejected as a feature request, since we generally avoid features that allow you to create consistent data between methods (eg city that matches the state).

@matthewmayer
Copy link
Contributor Author

I saw from the meeting notes you want to try to achieve this using patterns instead so it would work for more countries, but not sure how the current behavior could be replicated using string patterns as it includes numeric ranges.

@ST-DDT
Copy link
Member

ST-DDT commented Jul 31, 2023

I saw from the meeting notes you want to try to achieve this using patterns instead so it would work for more countries, but not sure how the current behavior could be replicated using string patterns as it includes numeric ranges.

0{{number.int(range)}}

@matthewmayer
Copy link
Contributor Author

have changed this to use string patterns passed to faker.helpers.fake

@Shinigami92
Copy link
Member

have changed this to use string patterns passed to faker.helpers.fake

Oh 😲 that is very interesting
We should discuss this in next meeting, but looks promising to me 👌

ST-DDT
ST-DDT previously approved these changes Jul 31, 2023
src/definitions/location.ts Show resolved Hide resolved
@ST-DDT ST-DDT added the s: needs decision Needs team/maintainer decision label Jul 31, 2023
@ST-DDT ST-DDT added the needs rebase There is a merge conflict label Aug 3, 2023
@ST-DDT
Copy link
Member

ST-DDT commented Aug 3, 2023

There seems to be a merge conflict.

@ST-DDT ST-DDT added s: accepted Accepted feature / Confirmed bug and removed needs rebase There is a merge conflict s: needs decision Needs team/maintainer decision labels Aug 3, 2023
@xDivisionByZerox xDivisionByZerox merged commit 0ca1e44 into faker-js:next Aug 10, 2023
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: bug Something isn't working m: location Something is referring to the location module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some en_US ZIP codes are only 4 characters when a state is specified
7 participants