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

location.zipCodeByState should throw for unknown states #1736

Closed
ST-DDT opened this issue Jan 14, 2023 · 7 comments
Closed

location.zipCodeByState should throw for unknown states #1736

ST-DDT opened this issue Jan 14, 2023 · 7 comments
Assignees
Labels
breaking change Cannot be merged when next version is not a major release c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs good first issue Good for newcomers m: location Something is referring to the location module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Milestone

Comments

@ST-DDT
Copy link
Member

ST-DDT commented Jan 14, 2023

Clear and concise description of the problem

Currently, if the given state is not known, then a random zip code is returned.
Usually, if I pass a value to a method, then I expect it to use it and tell me if the value is wrong.

Suggested solution

Throw if there is no data for that state.

Alternative

Keep the method as is.

Additional context

https://next.fakerjs.dev/api/location.html#zipcodebystate

@ST-DDT ST-DDT added good first issue Good for newcomers p: 1-normal Nothing urgent s: needs decision Needs team/maintainer decision c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs breaking change Cannot be merged when next version is not a major release m: location Something is referring to the location module labels Jan 14, 2023
@ST-DDT ST-DDT added this to the v8.0 - Module Re-Shuffling milestone Jan 14, 2023
@ST-DDT
Copy link
Member Author

ST-DDT commented Jan 19, 2023

Team decision

If the user explicitly calls zipCodeByState, then it should make sure that the state is actually valid.
-> throw error if the state is unknown.

@ST-DDT ST-DDT added s: accepted Accepted feature / Confirmed bug and removed s: needs decision Needs team/maintainer decision labels Jan 19, 2023
@Shinigami92
Copy link
Member

Shinigami92 commented Jan 20, 2023

Team decision

If the user explicitly calls zipCodeByState, then it should make sure that the state is actually valid. -> throw error if the state is unknown.

Sounds legit to me 👍


I move this to v8 milestone so we do not put more and more into v8.0
It's not blocked and if someone works on it, go for it, but if not, it can be a bug-fix later in v8.x

@ST-DDT
Copy link
Member Author

ST-DDT commented Jan 20, 2023

I move this to v8 milestone so we do not put more and more into v8.0
It's not blocked and if someone works on it, go for it, but if not, it can be a bug-fix later in v8.x

It would change documented api, so I consider this breaking:

If a locale does not have a postcode_by_state definition, a random zip code is generated according to the locale's zip format.

@ST-DDT
Copy link
Member Author

ST-DDT commented Jan 22, 2023

Pinging @0916dhkim to leave a comment hete so I can assign them to this issue for #1760 .

@0916dhkim
Copy link

👍

@xDivisionByZerox
Copy link
Member

If this is a breaking change, this needs to be put into the8.0 milestone no?

@ST-DDT
Copy link
Member Author

ST-DDT commented Feb 23, 2023

Team Decision

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Cannot be merged when next version is not a major release c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs good first issue Good for newcomers 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 a pull request may close this issue.

4 participants