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)!: zip code state #1874

Merged
merged 22 commits into from Apr 9, 2023
Merged

refactor(location)!: zip code state #1874

merged 22 commits into from Apr 9, 2023

Conversation

Shinigami92
Copy link
Member

references #1346 (comment)

@Shinigami92 Shinigami92 added 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 labels Feb 24, 2023
@Shinigami92 Shinigami92 self-assigned this Feb 24, 2023
@codecov
Copy link

codecov bot commented Feb 24, 2023

Codecov Report

Merging #1874 (b1b7b7d) into next (fdf7228) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #1874      +/-   ##
==========================================
- Coverage   99.62%   99.61%   -0.01%     
==========================================
  Files        2563     2563              
  Lines      243248   243278      +30     
  Branches     1282     1284       +2     
==========================================
+ Hits       242328   242353      +25     
- Misses        893      898       +5     
  Partials       27       27              
Impacted Files Coverage Δ
src/modules/location/index.ts 98.96% <100.00%> (+0.24%) ⬆️

... and 1 file with indirect coverage changes

@Shinigami92
Copy link
Member Author

@faker-js/maintainers @faker-js/members Is the current impl good enough? Or should we still throw a FakerError for missing state and make therefore a breaking change for the deprecated zipCodeForState?

@matthewmayer
Copy link
Contributor

matthewmayer commented Feb 24, 2023

I think in general breaking changes happening at the same time as deprecating a method is a bad idea. The point of deprecation is "you have one major version where your existing code will continue to work without changes".

@Shinigami92
Copy link
Member Author

I think in general breaking changes happening at the same time as deprecating a method is a bad idea. The point of deprecation is "you have one major version where your existing code will continue to work without changes".

Beside that: I'm asking because I don't see the need to throw a FakerError instead of doing a fallback 🤷
It just needs to be documented so if someone asks, we can point to the documentation 🙂

src/modules/location/index.ts Show resolved Hide resolved
src/modules/location/index.ts Show resolved Hide resolved
src/modules/location/index.ts Outdated Show resolved Hide resolved
@Shinigami92 Shinigami92 changed the title refactor(location): zip code state refactor(location)!: zip code state Mar 24, 2023
@Shinigami92 Shinigami92 added the breaking change Cannot be merged when next version is not a major release label Mar 24, 2023
test/location.spec.ts Outdated Show resolved Hide resolved
src/modules/location/index.ts Outdated Show resolved Hide resolved
src/modules/location/index.ts Outdated Show resolved Hide resolved
src/modules/location/index.ts Outdated Show resolved Hide resolved
src/modules/location/index.ts Show resolved Hide resolved
@Shinigami92
Copy link
Member Author

There is an issue that can be better identified by merging

first

@Shinigami92 Shinigami92 marked this pull request as ready for review April 2, 2023 10:47
@Shinigami92 Shinigami92 requested a review from a team as a code owner April 2, 2023 10:47
@Shinigami92 Shinigami92 requested review from ST-DDT and a team April 2, 2023 10:47
ST-DDT
ST-DDT previously approved these changes Apr 2, 2023
@ST-DDT ST-DDT requested a review from a team April 2, 2023 12:41
@ST-DDT ST-DDT added p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug labels Apr 4, 2023
@ST-DDT ST-DDT requested review from matthewmayer and a team April 4, 2023 20:38
Copy link
Member

@xDivisionByZerox xDivisionByZerox left a comment

Choose a reason for hiding this comment

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

My comment are non blocking. So feel free to resolve them if you don't think they are important.

test/location.spec.ts Show resolved Hide resolved
@Shinigami92
Copy link
Member Author

To proceed with this, I will merge it for now
We can extract issues or do several PRs like unifying the test variable names

@Shinigami92 Shinigami92 enabled auto-merge (squash) April 9, 2023 06:36
@Shinigami92 Shinigami92 merged commit 8574125 into next Apr 9, 2023
17 checks passed
@ST-DDT ST-DDT deleted the zip-code branch April 9, 2023 08:33
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 p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants