chore(locale): added curated names for default(en) locale #440

ganeshbudhathoki · 2022-02-05T23:00:07Z

ST-DDT

Could you please add the source of these lists to this PR?

test/name.spec.ts

Shinigami92 · 2022-02-06T12:21:35Z

src/name.ts

+      if (typeof gender === 'string') {
+        if (gender.toLowerCase() === 'male') {
+          gender = 0;
+        } else if (gender.toLowerCase() === 'female') {
+          gender = 1;
+        }
+      }
+
      if (typeof gender !== 'number') {
-        gender = this.faker.datatype.number(1);
+        if (typeof this.faker.definitions.name.middle_name === 'undefined') {
+          gender = this.faker.datatype.number(1);
+        } else {
+          // Fall back to unisex middle names if they exist and gender wasn't specified
+          return this.faker.random.arrayElement(
+            this.faker.definitions.name.middle_name
+          );
+        }


I think this code occurs multiple times in the name module, we should move it into a private helper method

@Shinigami92 I agree. Isolated the duplicated code into a private method. Let me know if it looks good.

ST-DDT · 2022-02-06T19:04:03Z

src/name.ts

+   * @example Name.determineGenderByStringParam('male') // 0
+   *
+   */
+  private static determineGenderByStringParam(gender: string | number) {


IMO this method should do most of the gender related stuff.
String handling, unknown number handling, undefined handling (choose random).
This method should also only ever return `0 | 1´.

I see what you are saying and I agree we should have methods to handle all that separately rather than handling those scenarios in every method but that touches every method. I think it should be a separate story/enhancement/chore to do that. This was to fix the default locale middle name bug.

There are three ways:

Just fix the middle name method without adding a new helper method.

Fix the middle name method by creating a separate method, but don't use it in the other methods yet.

Fix all name methods by creating a separate method and using it everywhere.

I suggest going for 1 or 2 and then creating a separate PR with option 3 later once this is merged.
@Shinigami92 Any preferences?

In the longtime goal, I would really like to deprecate numeric values for gender.
Why is male = 0 and female = 1? What's with binary genders?
The string values would not have a numeric order.

@griest024 also wanted to introduce the possibility to additionally use enum. #352

I would currently like to see just a switch case with default fallback.
But I'm not sure if this becomes to much for this PR and we may want to split it to another PR.
On the other side, as we already targeting 6.1 with this PR, maybe it's okay...

I would write it like so that we can call it this way:

export type Gender = ...; firstName(gender?: Gender): string return this.<functionName>(gender, this.faker.definitions.name.<arrayToUse>); }

I like the idea of Enum, but if we are choosing between lesser of the two evils, switch vs if, for now, I would stick with if, more concise and clear. Let me know your thoughts.

Non-technical nit: can we, like, literally remove the "gender binary" from our codebase (at some point)? I think we can still have the API, but I'd like to make an explicit effort to revamp everything surrounding gender in the lib. I feel pretty strongly about this.

Why is male = 0 and female = 1? What's with binary genders?

lol I love you Shini.

codecov · 2022-02-06T19:10:38Z

Codecov Report

Merging #440 (f26aebd) into main (1bc622a) will decrease coverage by 0.00%.
The diff coverage is 99.75%.

❗ Current head f26aebd differs from pull request most recent head 3ce0988. Consider uploading reports for the commit 3ce0988 to get more accurate results

@@            Coverage Diff             @@
##             main     #440      +/-   ##
==========================================
- Coverage   99.33%   99.32%   -0.01%     
==========================================
  Files        1920     1923       +3     
  Lines      176471   174617    -1854     
  Branches      905      896       -9     
==========================================
- Hits       175294   173443    -1851     
+ Misses       1121     1118       -3     
  Partials       56       56

Impacted Files	Coverage Δ
src/name.ts	`95.79% <96.87%> (+0.87%)`	⬆️
src/locales/en/name/female_middle_name.ts	`100.00% <100.00%> (ø)`
src/locales/en/name/index.ts	`100.00% <100.00%> (ø)`
src/locales/en/name/male_middle_name.ts	`100.00% <100.00%> (ø)`
src/locales/en/name/middle_name.ts	`100.00% <100.00%> (ø)`
src/vendor/user-agent.ts	`93.82% <0.00%> (-4.22%)`	⬇️
src/finance.ts	`99.33% <0.00%> (-0.67%)`	⬇️
src/internet.ts	`99.42% <0.00%> (-0.58%)`	⬇️
src/address.ts	`98.55% <0.00%> (-0.16%)`	⬇️
... and 463 more

Shinigami92 · 2022-03-10T19:40:21Z

@budaG This PR needs a rebase

Shinigami92 · 2022-03-21T14:10:34Z

I will revert all non-locale changes in this PR and create a PR on my own to fix this

fixes faker-js#373

…locale

ganeshbudhathoki requested a review from a team as a code owner February 5, 2022 23:00

ganeshbudhathoki mentioned this pull request Feb 5, 2022

faker.name lastName and middleName doesn't work correctly with gender selection #373

Closed

ST-DDT added the c: feature Request for new feature label Feb 6, 2022

ST-DDT added this to the v6.2 - New small features milestone Feb 6, 2022

ST-DDT assigned ganeshbudhathoki Feb 6, 2022

ST-DDT reviewed Feb 6, 2022

View reviewed changes

test/name.spec.ts Show resolved Hide resolved

ST-DDT modified the milestones: v6.2 - New small features, v6.1 - First bugfixes Feb 6, 2022

ST-DDT requested a review from a team February 6, 2022 09:26

Shinigami92 linked an issue Feb 6, 2022 that may be closed by this pull request

faker.name lastName and middleName doesn't work correctly with gender selection #373

Closed

Shinigami92 requested changes Feb 6, 2022

View reviewed changes

ST-DDT requested changes Feb 6, 2022

View reviewed changes

Shinigami92 mentioned this pull request Feb 23, 2022

Incorrect behavior of name.firstName() function #547

Closed

Shinigami92 added the needs rebase There is a merge conflict label Mar 10, 2022

Shinigami92 added c: bug Something isn't working p: 1-normal Nothing urgent c: locale Permutes locale definitions labels Mar 15, 2022

Shinigami92 changed the title ~~fix: added curated male, female & unisex middleName for default(en) locale (#373)~~ chore(locale): added curated male, female & unisex middleName for default(en) locale (#373) Mar 21, 2022

ganeshbudhathoki added 2 commits March 21, 2022 15:17

fix: added curated names for default(en) locale

90c509f

fixes faker-js#373

refactor: added helper method & enabled middleName testcase for 'uk' …

58e8168

…locale

Shinigami92 force-pushed the main branch from f26aebd to 58e8168 Compare March 21, 2022 14:19

chore(revert): name.ts changes

3ce0988

Shinigami92 approved these changes Mar 21, 2022

View reviewed changes

Shinigami92 requested a review from ST-DDT March 21, 2022 14:26

Shinigami92 added s: accepted Accepted feature / Confirmed bug and removed c: feature Request for new feature needs rebase There is a merge conflict labels Mar 21, 2022

ST-DDT approved these changes Mar 21, 2022

View reviewed changes

Shinigami92 changed the title ~~chore(locale): added curated male, female & unisex middleName for default(en) locale (#373)~~ chore(locale): added curated names for default(en) locale (#373) Mar 21, 2022

Shinigami92 enabled auto-merge March 21, 2022 14:27

Shinigami92 disabled auto-merge March 21, 2022 14:27

Shinigami92 changed the title ~~chore(locale): added curated names for default(en) locale (#373)~~ chore(locale): added curated names for default(en) locale Mar 21, 2022

Shinigami92 enabled auto-merge (squash) March 21, 2022 14:29

Shinigami92 merged commit 5642470 into faker-js:main Mar 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(locale): added curated names for default(en) locale #440

chore(locale): added curated names for default(en) locale #440

ganeshbudhathoki commented Feb 5, 2022

ST-DDT left a comment

Shinigami92 Feb 6, 2022

ganeshbudhathoki Feb 6, 2022

ST-DDT Feb 6, 2022

ganeshbudhathoki Feb 6, 2022

ST-DDT Feb 6, 2022 •

edited

Loading

Shinigami92 Feb 7, 2022

ganeshbudhathoki Feb 7, 2022

JessicaSachs Feb 8, 2022

JessicaSachs Feb 8, 2022

codecov bot commented Feb 6, 2022 •

edited

Loading

Shinigami92 commented Mar 10, 2022

Shinigami92 commented Mar 21, 2022

chore(locale): added curated names for default(en) locale #440

chore(locale): added curated names for default(en) locale #440

Conversation

ganeshbudhathoki commented Feb 5, 2022

ST-DDT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ST-DDT Feb 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 6, 2022 • edited Loading

Codecov Report

Shinigami92 commented Mar 10, 2022

Shinigami92 commented Mar 21, 2022

ST-DDT Feb 6, 2022 •

edited

Loading

codecov bot commented Feb 6, 2022 •

edited

Loading