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)!: throw error if state is unknown #1760

Closed
wants to merge 3 commits into from

Conversation

0916dhkim
Copy link

@0916dhkim 0916dhkim commented Jan 21, 2023

Copy link
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have trouble running the tests locally, you can use CI=true pnpm run test watch.

@@ -64,9 +64,9 @@ export class LocationModule {
const zipRange = this.faker.definitions.location.postcode_by_state?.[state];
if (zipRange) {
return String(this.faker.number.int(zipRange));
} else {
throw new Error(`${state} is invalid.`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please throw a FakerError instead.
Maybe also add a list with allowed keys and a hint where these keys are comming from.
E.g. faker.definitions.location.postcode_by_state only contains data for [...]

Copy link
Contributor

@matthewmayer matthewmayer Jan 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you might want to have different errors for the

  1. where the locale doesn't support postcode_by_state at all (e.g. en, de)
  2. where the locale does support postcode_by_state, but this is an unknown state (say XX in en_US).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm i think this is a weird edge case though where the fallback locale (en) doesn't contain the data, which is unusual.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ST-DDT Thanks for the review 🙏
I'll use FakeError instead + attach available keys.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have fixed the code to throw FakerError and updated the docstring, but I'm not sure how to make the tests pass. It looks like en locale is used as default in seededTests, and zipCodeByState will always throw with en.

@ST-DDT ST-DDT added c: bug Something isn't working p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug 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 and removed c: bug Something isn't working labels Jan 22, 2023
@ST-DDT ST-DDT added this to the v8.0 - Module Re-Shuffling milestone Jan 22, 2023
@ST-DDT ST-DDT added needs documentation Documentations are needed needs test More tests are needed labels Jan 22, 2023
Copy link
Contributor

@matthewmayer matthewmayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This documentation needs to be updated to reflect that an error would now be thrown. Also I think it could be make clearer this doesn't work in the default en locale, but only in en_US.

@Shinigami92 Shinigami92 changed the title refactor(location): Throw error if state is unknown refactor(location)!: throw error if state is unknown Feb 4, 2023
@Shinigami92 Shinigami92 added the needs rebase There is a merge conflict label Feb 4, 2023
@Shinigami92
Copy link
Member

The unresolved conflict brings an issue: the state parameter is required and therefore we have two options:

  1. We just allow zipCodeByState({ state: '...', .../* potential other options */ })
  2. We allow zipCodeByState({ state: '...', .../* potential other options */ }) as well as zipCodeByState('state', { /* potential other options */ })

Once we said we only want to have method calls (outside the helpers module) that can always be called without passing any arguments. In this case state is always required otherwise it would always throw an error.

We will discuss this in an upcoming meeting.

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

ST-DDT commented Feb 23, 2023

Team Decision


Thanks for your contribution anyway!

@ST-DDT ST-DDT closed this Feb 23, 2023
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 m: location Something is referring to the location module needs documentation Documentations are needed needs rebase There is a merge conflict needs test More tests are needed p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug s: needs decision Needs team/maintainer decision
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

location.zipCodeByState should throw for unknown states
4 participants