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

fix(person): add dedicated first names in ko locale #2773

Merged
merged 5 commits into from
Mar 27, 2024
Merged

Conversation

qqww08
Copy link
Contributor

@qqww08 qqww08 commented Mar 26, 2024

Fixed an issue where Korean and English were mixed.
And we added the latest trend names for 2022 and 2023.

fakerKO.person.fullName(). // 위 Blake

@qqww08 qqww08 requested a review from a team as a code owner March 26, 2024 09:32
Copy link

netlify bot commented Mar 26, 2024

Deploy Preview for fakerjs canceled.

Name Link
🔨 Latest commit c80c59c
🔍 Latest deploy log https://app.netlify.com/sites/fakerjs/deploys/6602c3e90fd0db0008a482ec

@xDivisionByZerox xDivisionByZerox added p: 1-normal Nothing urgent c: locale Permutes locale definitions m: person Something is referring to the person module labels Mar 26, 2024
@xDivisionByZerox xDivisionByZerox added this to the vAnytime milestone Mar 26, 2024
@xDivisionByZerox xDivisionByZerox requested a review from a team March 26, 2024 09:51
@xDivisionByZerox xDivisionByZerox changed the title fix(person): add ko locale fix(person): add dedicated first names in ko locale Mar 26, 2024
Copy link

codecov bot commented Mar 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.92%. Comparing base (7f11bf2) to head (c80c59c).

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #2773      +/-   ##
==========================================
- Coverage   99.93%   99.92%   -0.01%     
==========================================
  Files        2958     2960       +2     
  Lines      213686   211338    -2348     
  Branches      946      943       -3     
==========================================
- Hits       213549   211188    -2361     
- Misses        133      146      +13     
  Partials        4        4              
Files Coverage Δ
src/locales/ko/person/female_first_name.ts 100.00% <100.00%> (ø)
src/locales/ko/person/first_name.ts 100.00% <100.00%> (ø)
src/locales/ko/person/index.ts 100.00% <100.00%> (ø)
src/locales/ko/person/male_first_name.ts 100.00% <100.00%> (ø)
src/locales/ko/person/name.ts 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

@ST-DDT
Copy link
Member

ST-DDT commented Mar 26, 2024

Fixed an issue where Korean and English were mixed.

I didnt see any english names in the list.
Or do you refer to the ko translations of English names?
Or do you refer to the bug where firstName(sexType) return English names? #1677

And we added the latest trend names for 2022 and 2023.

This list seems to be significantly shorter than the previous one. Can we bump it to maybe a 100 each?

@qqww08
Copy link
Contributor Author

qqww08 commented Mar 26, 2024

@ST-DDT

There is no problem with firstName and lastName.

Try calling fakerKO.person.fullName()

I will update the list again with more names.

Shinigami92
Shinigami92 previously approved these changes Mar 26, 2024
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.

Would it be better to change src/locales/ko/person/name.ts to remove the space between the family name and given name ie

{ value: '{{person.lastName}} {{person.firstName}}', weight: 1 },

to

{ value: '{{person.lastName}}{{person.firstName}}', weight: 1 },

per https://en.wikipedia.org/wiki/Korean_name "When a Korean name is written in Hangul, there is no space between the surname and the given name."

@qqww08
Copy link
Contributor Author

qqww08 commented Mar 26, 2024

@ST-DDT

This list seems to be significantly shorter than the previous one. Can we bump it to maybe a 100 each?

We have added the ranking of the most used names in Korea from 1st to 300th.
https://efamily.scourt.go.kr/index.jsp

I referred to Korean government statistics.

Would it be better to change src/locales/ko/person/name.ts to remove the space between the family name and given name ie
per https://en.wikipedia.org/wiki/Korean_name "When a Korean name is written in Hangul, there is no space between the surname and the given name."

That's right, in Korea there are no spaces in the name. This was reflected immediately.

@ST-DDT ST-DDT merged commit 47f008a into faker-js:next Mar 27, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: locale Permutes locale definitions m: person Something is referring to the person module p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants