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

refactor(location): deprecate zipCode format parameter #3223

Open
wants to merge 8 commits into
base: next
Choose a base branch
from

Conversation

ST-DDT
Copy link
Member

@ST-DDT ST-DDT commented Oct 26, 2024

First part of #2390


Deprecates the format parameter in favor of using the native locale patterns or using faker.helpers.replaceSymbols directly.

@ST-DDT ST-DDT added p: 1-normal Nothing urgent c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs m: location Something is referring to the location module deprecation A deprecation was made in the PR labels Oct 26, 2024
@ST-DDT ST-DDT added this to the vAnytime milestone Oct 26, 2024
@ST-DDT ST-DDT self-assigned this Oct 26, 2024
@ST-DDT ST-DDT requested a review from a team as a code owner October 26, 2024 11:34
@ST-DDT ST-DDT requested a review from a team October 26, 2024 11:34
Copy link

netlify bot commented Oct 26, 2024

Deploy Preview for fakerjs ready!

Name Link
🔨 Latest commit 53c7198
🔍 Latest deploy log https://app.netlify.com/sites/fakerjs/deploys/673fb3b18a5f430008747915
😎 Deploy Preview https://deploy-preview-3223.fakerjs.dev
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Oct 26, 2024

Codecov Report

Attention: Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.

Project coverage is 99.96%. Comparing base (56acea3) to head (53c7198).

Files with missing lines Patch % Lines
src/modules/location/index.ts 94.11% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             next    #3223      +/-   ##
==========================================
- Coverage   99.97%   99.96%   -0.01%     
==========================================
  Files        2806     2806              
  Lines      217156   217168      +12     
  Branches      982      976       -6     
==========================================
- Hits       217092   217088       -4     
- Misses         64       80      +16     
Files with missing lines Coverage Δ
src/modules/location/index.ts 99.56% <94.11%> (-0.44%) ⬇️

... and 1 file with indirect coverage changes

---- 🚨 Try these New Features:

src/modules/location/index.ts Outdated Show resolved Hide resolved
@matthewmayer
Copy link
Contributor

Hmm

Given that the default en locale uses the patterns #####, #####-#### I would imagine that a very common case would be forcing 5 digit ZIPs using format:"#####"

We might want to provide better guidance for this scenario.

@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 30, 2024

Hmm

Given that the default en locale uses the patterns #####, #####-#### I would imagine that a very common case would be forcing 5 digit ZIPs using format:"#####"

We might want to provide better guidance for this scenario.

I'm not sure what you are asking for here.
Should I add an explicit example for zipCode('#####') to replaceSymbols('#####') or what?
IMO that is sufficiently covered by the deprecation message already:

https://github.com/faker-js/faker/blob/ebd0dd04a6055c8878f82c565d6ad13aa34ddd40/src/modules/location/index.ts#L55

I could change the runtime message to include the actual format parameter though:

- 'faker.location.zipCode(), faker.location.zipCode({ state }), or faker.helpers.replaceSymbols(format)',
+ `faker.location.zipCode(), faker.location.zipCode({ state }), or faker.helpers.replaceSymbols('${format}')`,

I hope the main usecase for that method is without the format parameter using the patterns provided by the locale.

Here are some calls using the format parameter.

And without format parameter

@matthewmayer
Copy link
Contributor

I think my issue is that zipCode("#####") is expressive in a way that none of the suggested replacements are

For example if I have code like

const address = [faker.location.streetAddress(), faker.location.city(), faker.location.state(), faker.location.zipCode("#####")].join("\n")

I feel to someone reading the code that's more intuitive than a call to say faker.string.numeric()

I think "I only want 5 digit zip codes" should be something the zip code method can handle.

@ST-DDT
Copy link
Member Author

ST-DDT commented Nov 3, 2024

One of the reasons I don't like that pattern is that it focuses on the wrong issue.
If you want to have a five digit postcode, why don't you use the one provided by your locale?
Does it have more than one variant? aka one short format and one long format?
Isn't the actual issue that there is no "format"/variant option to select between the two?

@matthewmayer
Copy link
Contributor

Yes agreed. Having an option where ZIP+4 is only used if an extended:true parameter is used might be good.

ZIP+4 is rarely used in practice in the US.

@ST-DDT
Copy link
Member Author

ST-DDT commented Nov 4, 2024

Yes agreed. Having an option where ZIP+4 is only used if an extended:true parameter is used might be good.

ZIP+4 is rarely used in practice in the US.

Unfortunately, I have no clue how the US zip code system works. Other than speeding up international mail delivery (SC) by a week/50% if I use a 9(?) digit zip code instead of the usual 5(?).

Does other countries have similar systems?

@matthewmayer
Copy link
Contributor

ChatGPT says:

Most Americans do not know their ZIP+4 code from memory and typically only use the standard five-digit ZIP code in everyday contexts. The ZIP+4 code system is primarily used by the USPS and businesses involved in bulk mailing or shipping, where accuracy and speed are essential. Here’s why ZIP+4 codes are not widely known or commonly used by the general public:

1.	Limited Necessity: For most personal mail and packages, the five-digit ZIP code is sufficient. USPS delivers accurately without requiring the extra four digits.
2.	Lack of Familiarity: ZIP+4 codes aren’t widely taught, and they’re not typically requested for most purposes, like filling out forms, so people rarely commit them to memory.
3.	Complexity: ZIP+4 codes can be specific to buildings, floors, or even particular departments within large companies or institutions. For individuals at residential addresses, the ZIP+4 may not be consistently applied or is often only referenced in specific settings.
4.	Automation: Online platforms and shipping services that benefit from ZIP+4 often auto-generate or auto-fill it during the address verification process. This eliminates the need for individuals to know or use their ZIP+4 manually.

In short, unless a ZIP+4 code is directly needed for business or government purposes, most people only remember and use the five-digit ZIP.

@ST-DDT
Copy link
Member Author

ST-DDT commented Nov 4, 2024

Do you know any other country that has multiple variants of zip codes?

@matthewmayer
Copy link
Contributor

Not really. In the UK for example postcodes are like "CB1 1DQ" and in some cases the first part "CB1" might be used to refer to an area like "central Cambridge ". But the default postcode is definitely the long/full one.

@ST-DDT
Copy link
Member Author

ST-DDT commented Nov 4, 2024

Would 'short' | 'long' be sufficient to distinguish between them?

I'll lookup our other locale's zip patterns for this as well.

@matthewmayer
Copy link
Contributor

The question is what is the default. For US zip we want short to be default. But in theory what if there's an another locale where the regular zip code is the long version but there's a short version which is a useful alternative.

@ST-DDT
Copy link
Member Author

ST-DDT commented Nov 4, 2024

The question is what is the default.

How about: The default is a random one. That way you can check how your system behaves with unexpected values. If you want a specific format you will look up the options and then pick one that you assume does the thing you need.

@ST-DDT ST-DDT added the s: needs decision Needs team/maintainer decision label Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs deprecation A deprecation was made in the PR m: location Something is referring to the location module p: 1-normal Nothing urgent s: needs decision Needs team/maintainer decision
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants