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

How to handle the incompatible types in address.zipCodeByState() #759

Closed
ST-DDT opened this issue Apr 2, 2022 · 3 comments · Fixed by #760
Closed

How to handle the incompatible types in address.zipCodeByState() #759

ST-DDT opened this issue Apr 2, 2022 · 3 comments · Fixed by #760
Assignees
Labels
c: locale Permutes locale definitions p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug

Comments

@ST-DDT
Copy link
Member

ST-DDT commented Apr 2, 2022

faker/src/address.ts

Lines 126 to 133 in f520968

zipCodeByState(state: string): string | number {
const zipRange = this.faker.definitions.address.postcode_by_state[state];
if (zipRange) {
// TODO ST-DDT 2022-02-10: Fix types
return this.faker.datatype.number(zipRange);
}
return this.faker.address.zipCode();
}

/**
* Postcodes patterns by state
*/
// TODO ST-DDT 2022-01-31: address.zipCodeByState() expects only { [state: string]: { min: number; max: number } }
postcode_by_state:
| string[]
| { [state: string]: { min: number; max: number } };

The following languages specify a string[] (4):

  • ar
  • en
  • he
  • ur

The following locales specify Parameters<Datatype['number']> (1):

  • en_US

AFAICT there is no way to access the string[] values using a string key and if there was a way the result would be incompatible with the datatype.number() function.

I checked the string[] postcode_by_state locale files and it is exactly like their postcode locale file, so even if we remove them, the fallback to the normal zipCode method would work and return the same values.

We have two choices:

  • Add support for string[] values
  • Remove the string[] from the type and the corresponding files

Created from #541

@ST-DDT ST-DDT added p: 1-normal Nothing urgent s: needs decision Needs team/maintainer decision c: locale Permutes locale definitions labels Apr 2, 2022
@ST-DDT ST-DDT added this to the v6.1 - First bugfixes milestone Apr 2, 2022
@ST-DDT
Copy link
Member Author

ST-DDT commented Apr 2, 2022

I'm in favor or removing string[] and the offending files.

@ST-DDT ST-DDT moved this to Todo in Faker Roadmap Apr 2, 2022
@xDivisionByZerox
Copy link
Member

After some investigation, I come to the same conclusion. A string[] doesn't make sense for the use case IMO.

@ST-DDT ST-DDT self-assigned this Apr 2, 2022
@ST-DDT ST-DDT added s: accepted Accepted feature / Confirmed bug and removed s: needs decision Needs team/maintainer decision labels Apr 2, 2022
@import-brain
Copy link
Member

I'm also in favor of removing string[].

@ST-DDT ST-DDT moved this from Todo to Awaiting Review in Faker Roadmap Apr 3, 2022
Repository owner moved this from Awaiting Review to Done in Faker Roadmap Apr 3, 2022
@ST-DDT ST-DDT removed this from Faker Roadmap Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: locale Permutes locale definitions p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants