Skip to content

feat(locale): update zh_CN location #3481

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Apr 19, 2025
Merged

feat(locale): update zh_CN location #3481

merged 4 commits into from
Apr 19, 2025

Conversation

yyz945947732
Copy link
Contributor

zh_CN location add city_name、continent、country、direction、language、secondary_address、street_name.

@yyz945947732 yyz945947732 requested a review from a team as a code owner April 15, 2025 08:33
@netlify Netlify
Copy link

netlify bot commented Apr 15, 2025 •
edited
Loading

✅ Deploy Preview for fakerjs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 31c9539
🔍 Latest deploy log https://app.netlify.com/sites/fakerjs/deploys/6803a9b3fedfcd00084038ec
😎 Deploy Preview https://deploy-preview-3481.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.

@Shinigami92 Shinigami92 added c: locale Permutes locale definitions m: location Something is referring to the location module labels Apr 15, 2025
@Shinigami92 Shinigami92 added this to the vAnytime milestone Apr 15, 2025
Shinigami92
Shinigami92 previously approved these changes Apr 15, 2025
View reviewed changes
@codecov Codecov
Copy link

codecov bot commented Apr 15, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.97%. Comparing base (cad8e5d) to head (31c9539).
Report is 10 commits behind head on next.

Additional details and impacted files 
@@            Coverage Diff             @@
##             next    #3481      +/-   ##
==========================================
- Coverage   99.97%   99.97%   -0.01%     
==========================================
  Files        2849     2856       +7     
  Lines      218992   219441     +449     
  Branches      952      949       -3     
==========================================
+ Hits       218939   219386     +447     
- Misses         53       55       +2
Files with missing lines Coverage Δ
src/locales/zh_CN/location/city_name.ts 100.00% <100.00%> (ø)
src/locales/zh_CN/location/continent.ts 100.00% <100.00%> (ø)
src/locales/zh_CN/location/country.ts 100.00% <100.00%> (ø)
src/locales/zh_CN/location/direction.ts 100.00% <100.00%> (ø)
src/locales/zh_CN/location/index.ts 100.00% <100.00%> (ø)
src/locales/zh_CN/location/language.ts 100.00% <100.00%> (ø)
src/locales/zh_CN/location/secondary_address.ts 100.00% <100.00%> (ø)
src/locales/zh_CN/location/street_name.ts 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

matthewmayer
matthewmayer previously approved these changes Apr 16, 2025
View reviewed changes
xDivisionByZerox
xDivisionByZerox reviewed Apr 18, 2025
View reviewed changes
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.

One last question from my side.
Could you also resolve the merge conflicts, please?

@yyz945947732 yyz945947732 dismissed stale reviews from matthewmayer and Shinigami92 via bca4236 April 19, 2025 13:07
@yyz945947732
Copy link
Contributor Author

Looks like every PR I submit modifies the test/__snapshots__/locale-data.spec.ts.snap file, so I have to resolve the conflicts one by one by following the steps below:

merge pr => sync fork => resolve confilcts => merge pr => sync fork => resolve confilcts .....

xDivisionByZerox
xDivisionByZerox approved these changes Apr 19, 2025
View reviewed changes
Copy link
Member

@xDivisionByZerox xDivisionByZerox left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks like every PR I submit modifies the test/__snapshots__/locale-data.spec.ts.snap file, so I have to resolve the conflicts one by one by following the steps below:

merge pr => sync fork => resolve confilcts => merge pr => sync fork => resolve confilcts .....

Your assumption is correct. Thank you for your awareness.
I suggest that I'll be writing a comment asking you for conflict resolution in the PR which I'll take on next. This way you do not need to "blindly" resolve merge conflicts multiple times in all your PRs.

I also noticed that you removed "翠竹路" from the street names to trigger the workflow actions again. While this is really kind of you, it's not strictly necessary since the failing test is a flaky one we can simply rerun the failing tests. Do you want to add the removed street name again or are you fine without it being in the data set? Both options are fine for me. As soon as you answer (and take action if you desire to do so) I'll merge this PR for you.

@yyz945947732
Copy link
Contributor Author

Your assumption is correct. Thank you for your awareness.
I suggest that I'll be writing a comment asking you for conflict resolution in the PR which I'll take on next. This way you do not need to "blindly" resolve merge conflicts multiple times in all your PRs.

That's great, thank you.

I also noticed that you removed "翠竹路" from the street names to trigger the workflow actions again. While this is really kind of you, it's not strictly necessary since the failing test is a flaky one we can simply rerun the failing tests. Do you want to add the removed street name again or are you fine without it being in the data set? Both options are fine for me. As soon as you answer (and take action if you desire to do so) I'll merge this PR for you.

Got it. I think it's fine to just remove this data, since it's not that important.

@xDivisionByZerox
Copy link
Member

In that case I'll already merge this PR. It was previously approved so I'm dismissing the two approval rule this time.

@xDivisionByZerox xDivisionByZerox merged commit 456f102 into faker-js:next Apr 19, 2025
23 checks passed
@xDivisionByZerox xDivisionByZerox added the c: feature Request for new feature label May 4, 2025
@xDivisionByZerox xDivisionByZerox modified the milestones: vAnytime, v9.x May 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xDivisionByZerox xDivisionByZerox xDivisionByZerox approved these changes

@Shinigami92 Shinigami92 Awaiting requested review from Shinigami92

@matthewmayer matthewmayer Awaiting requested review from matthewmayer

Assignees

@yyz945947732 yyz945947732

Labels
c: feature Request for new feature c: locale Permutes locale definitions m: location Something is referring to the location module
Projects
None yet
Milestone
v9.x
Development

Successfully merging this pull request may close these issues.
4 participants
@yyz945947732 @xDivisionByZerox @matthewmayer @Shinigami92