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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 26 commits into from
Aug 19, 2022
Merged

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

merged 26 commits into from
Aug 19, 2022

Conversation

hankucz
Copy link
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 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

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
src/modules/name/index.ts Show resolved Hide resolved
src/modules/name/index.ts Show resolved Hide resolved
Shinigami92
Shinigami92 reviewed Jul 21, 2022
View reviewed changes
Copy link
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
Member

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
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
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
Member

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

@Shinigami92 Shinigami92 Shinigami92 approved these changes

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

@pkuczynski pkuczynski pkuczynski approved these changes

@xDivisionByZerox xDivisionByZerox Awaiting requested review from xDivisionByZerox

@JessicaSachs JessicaSachs Awaiting requested review from JessicaSachs

@import-brain import-brain Awaiting requested review from import-brain

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 @import-brain