Skip to content

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

Merged
ST-DDT merged 5 commits into
faker-js:nextfrom
qqww08:next
Mar 27, 2024
Merged

fix(person): add dedicated first names in ko locale#2773
ST-DDT merged 5 commits into
faker-js:nextfrom
qqww08:next

Conversation

@qqww08

@qqww08 qqww08 commented Mar 26, 2024

Copy link
Copy Markdown
Contributor

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
@netlify

netlify Bot commented Mar 26, 2024

Copy link
Copy Markdown

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
@codecov

codecov Bot commented Mar 26, 2024

Copy link
Copy Markdown

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

ST-DDT commented Mar 26, 2024

Copy link
Copy Markdown
Member

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

qqww08 commented Mar 26, 2024

Copy link
Copy Markdown
Contributor Author

@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

@matthewmayer matthewmayer left a comment

Copy link
Copy Markdown
Contributor

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 qqww08 dismissed stale reviews from Shinigami92 and xDivisionByZerox via 7777aab March 26, 2024 12:39
@qqww08

qqww08 commented Mar 26, 2024

Copy link
Copy Markdown
Contributor Author

@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
@ST-DDT ST-DDT modified the milestones: vAnytime, v9.0 Jun 22, 2026
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

5 participants