Skip to content

feat(name): extract sex generator from gender to sex#1168

Merged
Shinigami92 merged 26 commits intofaker-js:mainfrom
hankucz:binary-gender-2-sex
Aug 19, 2022
Merged

feat(name): extract sex generator from gender to sex#1168
Shinigami92 merged 26 commits intofaker-js:mainfrom
hankucz:binary-gender-2-sex

Conversation

@hankucz
Copy link
Copy Markdown
Contributor

@hankucz hankucz commented Jul 19, 2022 •
edited by pkuczynski
Loading

Fixes #353
Fixes #776

  • extract sex generator from name.gender to name.sex
  • deprecate name.gender(true), which generated binary gender, what we would consider now as sex
  • rename locale definitions from binary_gender to sex
  • lowercase english values for sex
  • extend english locale with non-binary

@hankucz hankucz requested a review from a team as a code owner July 19, 2022 11:42
xDivisionByZerox
xDivisionByZerox reviewed Jul 19, 2022
View reviewed changes
Comment thread src/definitions/name.ts
xDivisionByZerox and others added 3 commits July 19, 2022 13:57
@xDivisionByZerox @hankucz
refactor(name.findName): rename to fullName (faker-js#1127)
8b41348
@xDivisionByZerox @pkuczynski @hankucz
feat(system.fileName): file extension count (faker-js#1101)
a748bbc 
Co-authored-by: Piotr Kuczynski <piotr.kuczynski@gmail.com>
@renovate @hankucz
chore(deps): update doc-dependencies (faker-js#1160)
7d360d7 
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
@xDivisionByZerox xDivisionByZerox added c: locale Permutes locale definitions c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs s: accepted Accepted feature / Confirmed bug p: 1-normal Nothing urgent labels Jul 19, 2022
@xDivisionByZerox xDivisionByZerox added this to the v7 - Current Major milestone Jul 19, 2022
@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 19, 2022 •
edited
Loading

Codecov Report

Merging #1168 (ca21e20) into main (ca7cb41) will increase coverage by 0.00%.
The diff coverage is 97.77%.

@@           Coverage Diff           @@
##             main    #1168   +/-   ##
=======================================
  Coverage   99.62%   99.62%           
=======================================
  Files        2158     2159    +1     
  Lines      240193   240217   +24     
  Branches     1003     1005    +2     
=======================================
+ Hits       239294   239325   +31     
+ Misses        878      871    -7     
  Partials       21       21
Impacted Files Coverage Δ
src/definitions/name.ts 0.00% <0.00%> (ø)
src/locales/fr/name/sex.ts 100.00% <ø> (ø)
src/locales/fr_CH/name/sex.ts 100.00% <ø> (ø)
src/locales/pt_BR/name/sex.ts 100.00% <ø> (ø)
src/locales/ur/name/sex.ts 100.00% <ø> (ø)
src/locales/de/name/index.ts 100.00% <100.00%> (ø)
src/locales/de/name/sex.ts 100.00% <100.00%> (ø)
src/locales/en/name/index.ts 100.00% <100.00%> (ø)
src/locales/en/name/sex.ts 100.00% <100.00%> (ø)
src/locales/fr/name/index.ts 100.00% <100.00%> (ø)
... and 7 more

ST-DDT
ST-DDT requested changes Jul 20, 2022
View reviewed changes
Comment thread src/modules/name/index.ts
Comment thread src/modules/name/index.ts
Shinigami92
Shinigami92 reviewed Jul 21, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

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

This is a breaking change as binary_gender could be used in faker.faker('{{binary_gender}}') without that we know it or users know that it had changed
I mark this as blocked for now and would like to first discuss if this PR is needed anyway

@Shinigami92 Shinigami92 added s: on hold Blocked by something or frozen to avoid conflicts breaking change Cannot be merged when next version is not a major release and removed s: accepted Accepted feature / Confirmed bug labels Jul 21, 2022
@pkuczynski
Copy link
Copy Markdown
Member

pkuczynski commented Jul 26, 2022

This is a breaking change as binary_gender could be used in faker.faker('{{binary_gender}}') without that we know it or users know that it had changed
I mark this as blocked for now and would like to first discuss if this PR is needed anyway

If we are killing gender enum, it feels like this is a natural good step forward to make the differentiation between gender and sex.

In my opinion faker.fake should be deprecated, as the same result can be achieved with string templates, which additionally allow user to provide options to method calls, instead of just using locales.

So yes, technically it could be a breaking change, but at the same time locale formats are not officially published, so I have doubts if anybody would hook up to the internals so deep to find it. And if he did, it's kind of not our fault...

@ST-DDT @xDivisionByZerox wdyt?

hankucz added 2 commits July 26, 2022 19:48
@hankucz
Merge branch 'main' into binary-gender-2-sex
09a6810 
# Conflicts:
#	test/__snapshots__/name.spec.ts.snap
#	test/name.spec.ts
@hankucz
fix: tests
b2f97a6
@hankucz
Copy link
Copy Markdown
Contributor Author

hankucz commented Aug 18, 2022

I believe I applied all of the feedback. Is there anything else I should do about it?

@Shinigami92
Copy link
Copy Markdown
Member

Shinigami92 commented Aug 19, 2022

Ping @ST-DDT, you need to approve to stale your requested changes

@Shinigami92 Shinigami92 merged commit ad3c9bf into faker-js:main Aug 19, 2022
@xDivisionByZerox xDivisionByZerox mentioned this pull request Aug 26, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pkuczynski pkuczynski pkuczynski approved these changes

@ST-DDT ST-DDT ST-DDT approved these changes

@Shinigami92 Shinigami92 Shinigami92 approved these changes

@xDivisionByZerox xDivisionByZerox Awaiting requested review from xDivisionByZerox

@JessicaSachs JessicaSachs Awaiting requested review from JessicaSachs

@ejcheng ejcheng Awaiting requested review from ejcheng

Assignees

@hankucz hankucz

Labels

breaking change Cannot be merged when next version is not a major release c: locale Permutes locale definitions c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs deprecation A deprecation was made in the PR m: person Something is referring to the person module p: 1-normal Nothing urgent s: on hold Blocked by something or frozen to avoid conflicts

Projects

No open projects
Faker Roadmap
Status: Done

Milestone

v7 - Current Major

Development

Successfully merging this pull request may close these issues.

Typing of gender conflicts with common usage patterns Clarify usages of sex vs gender

7 participants

@hankucz @pkuczynski @xDivisionByZerox @Shinigami92 @ST-DDT @JessicaSachs @ejcheng