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): Incorrect behavior of name.firstName() function #1610

Merged
merged 10 commits into from
Dec 21, 2022

Conversation

matthewmayer
Copy link
Contributor

@matthewmayer matthewmayer commented Nov 27, 2022

fix #547 for az,id_ID, lv, ro, ru, sk, uk locales
fix #1553 for de_AT, nl_BE, en_IN, en_AU and el locales

@codecov
Copy link

codecov bot commented Nov 27, 2022

Codecov Report

Merging #1610 (86af798) into next (2d0ca3f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff            @@
##             next    #1610    +/-   ##
========================================
  Coverage   99.64%   99.65%            
========================================
  Files        2227     2244    +17     
  Lines      240254   240413   +159     
  Branches     1070     1073     +3     
========================================
+ Hits       239396   239578   +182     
+ Misses        837      814    -23     
  Partials       21       21            
Impacted Files Coverage Δ
src/locales/az/person/first_name.ts 100.00% <100.00%> (ø)
src/locales/az/person/index.ts 100.00% <100.00%> (ø)
src/locales/de_AT/person/female_first_name.ts 100.00% <100.00%> (ø)
src/locales/de_AT/person/first_name.ts 100.00% <100.00%> (ø)
src/locales/de_AT/person/index.ts 100.00% <100.00%> (ø)
src/locales/de_AT/person/male_first_name.ts 100.00% <100.00%> (ø)
src/locales/el/person/female_first_name.ts 100.00% <100.00%> (ø)
src/locales/el/person/first_name.ts 100.00% <100.00%> (ø)
src/locales/el/person/index.ts 100.00% <100.00%> (ø)
src/locales/el/person/male_first_name.ts 100.00% <100.00%> (ø)
... and 26 more

Shinigami92
Shinigami92 previously approved these changes Nov 27, 2022
ST-DDT
ST-DDT previously approved these changes Nov 27, 2022
Copy link
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

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

It doesnt fix the underlying issue, but IMO it fixes the symptoms so it is good enough.

We have to treat the underlying issue later though (in a new issue if we havent one already).

…gendered first_name for nl_BE, en_IN, en_AU and de_AT
@matthewmayer matthewmayer dismissed stale reviews from ST-DDT and Shinigami92 via 201d4f8 November 27, 2022 15:47
@matthewmayer
Copy link
Contributor Author

For the four locales de_AT, nl_BE, en_IN and en_AU it was straightforward to split the first_name.ts file into two gendered files. This solves this problem in Faker 7.x:

> faker.setLocale("de_AT")
> faker.name.firstName()
'Leandra' //ok, this is from de_AT
> faker.name.firstName("female")
'Brenda' //wrong, this is from en

@matthewmayer
Copy link
Contributor Author

matthewmayer commented Nov 27, 2022

For el, all the names in first_name.ts were actually male first names! So I made that male_first_name and added some new female_first_names based on popular first names from statistics.gr

> faker.setLocale("el")
> faker.person.firstName()
'Διονύσιος'
> faker.person.firstName("female")
'Αννα'
> faker.person.firstName("male")
'Παρασκευάς'

@matthewmayer
Copy link
Contributor Author

For the remaining locales the names are either intermingled or I don't know the languages well enough to tell, so this will have to wait until later refactoring:

de_CH
es_MX
ge
ko
ne
zh_TW

ST-DDT
ST-DDT previously approved these changes Nov 27, 2022
@ST-DDT ST-DDT added p: 1-normal Nothing urgent c: locale Permutes locale definitions m: person Something is referring to the person module labels Nov 27, 2022
@ST-DDT ST-DDT added the s: accepted Accepted feature / Confirmed bug label Dec 8, 2022
@ST-DDT ST-DDT requested review from Shinigami92 and a team December 11, 2022 01:11
@ST-DDT
Copy link
Member

ST-DDT commented Dec 18, 2022

Just to be sure, did you only use the mergeArrays function if the "generic" name was exactly the union of "female" and "male"?

Nevermind. I checked myself. The files were either missing before or are matching the union.

@ST-DDT ST-DDT enabled auto-merge (squash) December 21, 2022 01:45
@ST-DDT ST-DDT merged commit 4ce8e98 into faker-js:next Dec 21, 2022
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 s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

fullName uses fallback first names in el (Greek) Incorrect behavior of name.firstName() function
4 participants