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

feat(name): add full_name_pattern configuration #1211 #1216

Closed
wants to merge 14 commits into from
Closed

feat(name): add full_name_pattern configuration #1211 #1216

wants to merge 14 commits into from

Conversation

satoc0629
Copy link
Contributor

No description provided.

@satoc0629 satoc0629 requested a review from a team as a code owner July 31, 2022 08:41
@ST-DDT
Copy link
Member

ST-DDT commented Jul 31, 2022

Could you please provide the male and female first names in a separate PR?
We would like to keep locale changes separate from implementation changes/new feature to simplify the review process.

@ST-DDT ST-DDT added c: feature Request for new feature p: 1-normal Nothing urgent m: person Something is referring to the person module labels Jul 31, 2022
src/modules/name/index.ts Outdated Show resolved Hide resolved
src/definitions/name.ts Outdated Show resolved Hide resolved
src/definitions/name.ts Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 31, 2022

Codecov Report

Merging #1216 (073140a) into main (eb63f91) will decrease coverage by 0.00%.
The diff coverage is 91.80%.

@@            Coverage Diff             @@
##             main    #1216      +/-   ##
==========================================
- Coverage   99.62%   99.62%   -0.01%     
==========================================
  Files        2163     2165       +2     
  Lines      241264   241318      +54     
  Branches     1014     1028      +14     
==========================================
+ Hits       240357   240406      +49     
- Misses        886      891       +5     
  Partials       21       21              
Impacted Files Coverage Δ
src/definitions/name.ts 0.00% <0.00%> (ø)
src/modules/name/index.ts 99.30% <92.10%> (-0.70%) ⬇️
src/locales/ja/name/name.ts 100.00% <100.00%> (ø)
src/locales/mk/name/female_name.ts 100.00% <100.00%> (ø)
src/locales/mk/name/index.ts 100.00% <100.00%> (ø)
src/locales/mk/name/male_name.ts 100.00% <100.00%> (ø)

@satoc0629
Copy link
Contributor Author

別の PR で男性と女性の名前を教えてください。 レビュー プロセスを簡素化するために、ロケールの変更を実装の変更/新機能とは別にしたいと考えています。

We have responded here.

@satoc0629

This comment was marked as duplicate.

src/modules/name/index.ts Outdated Show resolved Hide resolved
@ST-DDT ST-DDT linked an issue Aug 4, 2022 that may be closed by this pull request
@satoc0629 satoc0629 requested a review from Shinigami92 August 19, 2022 12:32
@satoc0629 satoc0629 requested review from ST-DDT and Shinigami92 and removed request for Shinigami92 and ST-DDT September 17, 2022 07:53
@ST-DDT
Copy link
Member

ST-DDT commented Sep 17, 2022

The full name pattern isnt used in the code and if it will be it should be done constently for all locales

@satoc0629
Copy link
Contributor Author

@ST-DDT

The full name pattern isnt used in the code and if it will be it should be done constently for all locales

In the most recent commit, the full_name_pattern method was abandoned.
On the contrary, we are changing to a method that refers to the existing legacy asset, the name template.

The name template is located in src/definitions/name.ts.

  /**
   * A list of patterns used to generate names.
   */
  name: string[];

@ST-DDT
Copy link
Member

ST-DDT commented Sep 17, 2022

You seem to have missed one file: src/locales/ja/name/full_name_pattern.ts

@satoc0629
Copy link
Contributor Author

@ST-DDT
Sorry. I pushed one new commit.

@satoc0629
Copy link
Contributor Author

@ST-DDT

In addition to src/definitions/name.ts
src/definitions/female_name.ts and src/definitions/male_name.ts can now be set.
This allows for the conventional behavior of using src/definitions/name.ts as the default, as well as the use of the above when sex is specified.

@ST-DDT ST-DDT changed the title Add full_name_pattern configuration #1211 feat(name): add full_name_pattern configuration #1211 Sep 18, 2022
return defaultName;
}

const fullNamePattern = this.faker.helpers.arrayElement(
Copy link
Member

Choose a reason for hiding this comment

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

The selectDefinition method at the top should do the same in a more readable way..

'name.male_last_name': () =>
lastName ? lastName : this.lastName('male'),
'name.suffix': () => (suffix ? suffix : this.suffix()),
});
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of this, as this hard to maintain/extend.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ST-DDT
Is this an implementation issue?
Or is it an idea problem?
Thank you.

Copy link
Member

@ST-DDT ST-DDT Sep 18, 2022

Choose a reason for hiding this comment

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

Yes, it would be nice of you could at least remove/merge the male/female parts and then call it with the (pregenerated) gender option.
Or maybe, doing a faker.mustache with the method names, followed by a faker.fake with the locale keys.
Not sure what "feels" best.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ST-DDT
Please take a look at the following commit.
073140a

@ST-DDT ST-DDT added the needs rebase There is a merge conflict label Nov 21, 2022
@ST-DDT
Copy link
Member

ST-DDT commented Jan 26, 2023

Superseded by #1637

@ST-DDT ST-DDT closed this Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: feature Request for new feature m: person Something is referring to the person module needs rebase There is a merge conflict p: 1-normal Nothing urgent
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add the concept of order for firstName and lastName in name.fullName
3 participants