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(locale): limited ja first names without passing sex #2190

Merged

Conversation

KeisukeYamashita
Copy link
Contributor

@KeisukeYamashita KeisukeYamashita commented May 31, 2023

Hello, I have noticed that ja person first name only has 21 entries while female_first_name has more than 100 and male_first_name as well.

Even though hard work has done in each sex is defined, if nothing is specified, only a limited number of options will be returned. So I have merged the two arrays as first_name and now the first_name() function can return from almost 279 candidates (258 increase 🚀 ).

Moreover, this avoids duplicate management of each list separately, making them easier to manage.

Reference

This is exactly the same way as some other languages, for example:

fr

import { mergeArrays } from './../../../internal/merge';
import female_prefix from './female_prefix';
import male_prefix from './male_prefix';
export default mergeArrays(female_prefix, male_prefix);

ne

import { mergeArrays } from './../../../internal/merge';
import female_first_name from './female_first_name';
import male_first_name from './male_first_name';
export default mergeArrays(female_first_name, male_first_name);

Signed-off-by: KeisukeYamashita <19yamashita15@gmail.com>
@KeisukeYamashita KeisukeYamashita marked this pull request as ready for review May 31, 2023 05:37
@KeisukeYamashita KeisukeYamashita requested a review from a team as a code owner May 31, 2023 05:38
@codecov
Copy link

codecov bot commented May 31, 2023

Codecov Report

Merging #2190 (0508a6a) into next (cdd162a) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 0508a6a differs from pull request most recent head 07a5a3e. Consider uploading reports for the commit 07a5a3e to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #2190      +/-   ##
==========================================
- Coverage   99.60%   99.59%   -0.01%     
==========================================
  Files        2607     2607              
  Lines      245021   245014       -7     
  Branches     1154     1151       -3     
==========================================
- Hits       244043   244021      -22     
- Misses        951      966      +15     
  Partials       27       27              
Impacted Files Coverage Δ
src/locales/ja/person/first_name.ts 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

@matthewmayer matthewmayer added c: locale Permutes locale definitions m: person Something is referring to the person module labels May 31, 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.

LGTM. Having most locales using the mergeArray function will simplify things when we restructure the gendered definitions.

@xDivisionByZerox
Copy link
Member

@KeisukeYamashita Just to be sure and for future reference: Does the concept of gender-neutral first names not exist in Japanese or where the names in the first_name.ts file simply not gender-neutral?

@ST-DDT ST-DDT added the p: 1-normal Nothing urgent label Jun 1, 2023
@KeisukeYamashita
Copy link
Contributor Author

KeisukeYamashita commented Jun 2, 2023

@xDivisionByZerox Thank you for your comment!

In Japan, AFAK、there are some gender neutral names like アオイ、レイ、etc. It's can be used for male and female names.

Perhaps I was mistaken, but I thought the firstName function returned either male or female names (including some gender-neutral names), not only gender-neutral names.

I thought the situation was similar in other countries, so I used mergeArray as a reference.

@ST-DDT ST-DDT changed the title fix(locale): limited ja first names candidates without passing sex fix(locale): limited ja first names without passing sex Jun 4, 2023
@ST-DDT ST-DDT enabled auto-merge (squash) June 4, 2023 21:58
@ST-DDT ST-DDT merged commit 5de8874 into faker-js:next Jun 4, 2023
19 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
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants