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): delete set of invalid names in en_NG #2764

Merged
merged 1 commit into from
Mar 22, 2024

Conversation

nbroad1881
Copy link
Contributor

These names don't seem legitimate

These names don't seem legitimate
@nbroad1881 nbroad1881 requested a review from a team as a code owner March 22, 2024 00:41
Copy link

netlify bot commented Mar 22, 2024

👷 Deploy request for fakerjs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 87deb18

@matthewmayer matthewmayer changed the title Delete names fix(person): Delete names in en_NG Mar 22, 2024
Copy link

codecov bot commented Mar 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.93%. Comparing base (a27aafe) to head (87deb18).

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #2764      +/-   ##
==========================================
- Coverage   99.94%   99.93%   -0.01%     
==========================================
  Files        2958     2958              
  Lines      213715   213713       -2     
  Branches      603      950     +347     
==========================================
- Hits       213595   213583      -12     
- Misses        120      130      +10     
Files Coverage Δ
src/locales/en_GH/person/male_first_name.ts 100.00% <ø> (ø)

... and 1 file with indirect coverage changes

@matthewmayer
Copy link
Contributor

It seems these may be literal translations of Nigerian names in other languages eg eg Daluchi in Igbo

https://www.legit.ng/ask-legit/quotes-messages/1501704-powerful-nigerian-names-meanings-fascinate-you/ but I couldn't find much evidence people use these translations in English.

@xDivisionByZerox xDivisionByZerox added p: 1-normal Nothing urgent c: locale Permutes locale definitions m: person Something is referring to the person module labels Mar 22, 2024
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.

Works for me.

@xDivisionByZerox xDivisionByZerox changed the title fix(person): Delete names in en_NG fix(person): delete set of invalid names in en_NG Mar 22, 2024
@xDivisionByZerox xDivisionByZerox requested review from a team March 22, 2024 09:48
@xDivisionByZerox xDivisionByZerox added this to the vAnytime milestone Mar 22, 2024
@matthewmayer
Copy link
Contributor

Hmm at some point this was a different list with fewer traditional English names

https://github.com/faker-js/faker/blob/7dc1bc0609b7d7865835cabed86c30e239c357c8/lib/locales/en_NG/name/male_first_name.js

@ST-DDT ST-DDT requested a review from a team March 22, 2024 10:12
@matthewmayer
Copy link
Contributor

OK i found the original commit:

fb62766

@prinx if you are still around maybe you can help verify this.

@ST-DDT
Copy link
Member

ST-DDT commented Mar 22, 2024

Hmm at some point this was a different list with fewer traditional English names

https://github.com/faker-js/faker/blob/7dc1bc0609b7d7865835cabed86c30e239c357c8/lib/locales/en_NG/name/male_first_name.js

If you look those up please also include the commit id that added/removed the other data.

Then I can try to lookup the original PR that changed these.

If @nbroad1881 confirms that the other names are fine/used in en_ng, then we can merge this.

@ST-DDT
Copy link
Member

ST-DDT commented Mar 22, 2024

OK i found the original commit:

fb62766

Merged in f694a2e

Original PR discussion (as far as recoverable):
Author Time Comment
prinx 2021-03-09 05:46:27 While testing I ran the the `browser` script, which overwrote the files in the `examples` and `dist` folders.
Marak 2021-03-09 14:05:00 We can't accept pull requests that modify `/dist` or `examples`
prinx 2021-03-09 14:22:51 Yes, I was wondering. Thanks. will revert the changes and create a PR
prinx 2021-03-10 22:37:34 Hi @Marak . FYI.
Marak 2021-03-11 15:20:21 While testing I ran the the `browser` script, which overwrote the files in the `examples` and `dist` folders.
Marak 2021-03-11 15:20:30 Thank you for your contributions @prinx Merged!
prinx 2021-03-11 17:11:12 You are welcome @Marak . Thanks for merging too. Hoping to contribute more.

No more (recoverable) contributions or comments to FakerJs by prinx after that.

@prinx
Copy link
Contributor

prinx commented Mar 22, 2024

OK i found the original commit:

fb62766

@prinx if you are still around maybe you can help verify this.

Yes, those names are literal translations of local names to English but are not used in English as stated. They can be removed. Thanks for spotting that.

@ST-DDT ST-DDT merged commit 2884552 into faker-js:next Mar 22, 2024
22 of 23 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

6 participants