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

chore: rename Gender to Sex #1163

Merged
merged 13 commits into from
Aug 1, 2022
Merged

chore: rename Gender to Sex #1163

merged 13 commits into from
Aug 1, 2022

Conversation

hankucz
Copy link
Contributor

@hankucz hankucz commented Jul 18, 2022

Fixes #352

Questions:

  • I don't know how to deprecate Gender type in favor of Sex
  • I haven't added Intersex nor Nonbinary to the enum, as I felt that the way it is used, it would be better not to specify the sex, which would lead to randomizing the value despite the sex. For example 'faker.name.firstName()` would provide either a female or male first name, which IMHO would provide inclusivity for non-binary data...
  • I could also create a separate category of names, which are not sex-specific, like Alex, Max, Sam, etc and then I could add a new enum value for picking them up. Although some (or maybe even many) languages do not have such names (eg. in Polish, every female name must end with an a, and if a name does not, it is considered male - there are no exceptions to this rule).
  • Some other fixes in this module will follow in a separate PR, for example Use enum for male/female references #352

@hankucz hankucz requested a review from a team as a code owner July 18, 2022 20:26
@codecov
Copy link

codecov bot commented Jul 18, 2022

Codecov Report

Merging #1163 (f746d11) into main (4fa243e) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

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

@@            Coverage Diff             @@
##             main    #1163      +/-   ##
==========================================
- Coverage   99.62%   99.62%   -0.01%     
==========================================
  Files        2153     2153              
  Lines      236521   236547      +26     
  Branches      979      980       +1     
==========================================
+ Hits       235643   235659      +16     
- Misses        857      867      +10     
  Partials       21       21              
Impacted Files Coverage Δ
src/index.ts 100.00% <100.00%> (ø)
src/modules/name/index.ts 100.00% <100.00%> (ø)
src/modules/internet/user-agent.ts 81.74% <0.00%> (-2.65%) ⬇️

@xDivisionByZerox
Copy link
Member

I don't know how to deprecate Gender type in favor of Sex

As far as I can tell there is no way to do that other than documentation since the type will be gone as soon as the TypeScript code is compiled to JavaScript.

I haven't added Intersex nor Nonbinary to the enum, as I felt that the way it is used, it would be better not to specify the sex, which would lead to randomizing the value despite the sex. For example 'faker.name.firstName()` would provide either a female or male first name, which IMHO would provide inclusivity for non-binary data...

I could also create a separate category of names, which are not sex-specific, like Alex, Max, Sam, etc and then I could add a new enum value for picking them up. Although some (or maybe even many) languages do not have such names (eg. in Polish, every female name must end with an a, and if a name does not, it is considered male - there are no exceptions to this rule).

We have already talked about this possibility in a team meeting. But, you also came to the conclusion that it's not as easy as just providing an extra set of names to pick from (your Polish example). Because of that, we decided to keep the options as they are, for now. When the library is considered stable and easier maintainable we will think about such features.

@xDivisionByZerox
Copy link
Member

The current change would be considered a breaking change since the Gender enum would be removed. But like I said in an earlier comment, I'm not sure if you can deprecate and enum. I'm just pinging @Shinigami92 and @ST-DDT so they will see this (but they are currently on vacation).

One other think is that I'd like to get the additional type (now SexValue) removed. But this doesn't belong in this PR. Just leaving this here for reference.

Otherwise this looks good to me.

src/index.ts Show resolved Hide resolved
src/modules/name/index.ts Show resolved Hide resolved
src/modules/name/index.ts Outdated Show resolved Hide resolved
src/modules/name/index.ts Outdated Show resolved Hide resolved
@hankucz
Copy link
Contributor Author

hankucz commented Jul 19, 2022

The current change would be considered a breaking change since the Gender enum would be removed. But like I said in an earlier comment, I'm not sure if you can deprecate and enum.

I am not sure if anybody would use the enum, as initially, I changed types to use it, and all tests were failing compilation when called with strings like firstName('female'), which is how they all were documented. I was thinking whether it makes sense to export the enum at all, and maybe only use the string type? Eventually, I decided to go with both but export only enum. What do you think?

One other think is that I'd like to get the additional type (now SexValue) removed. But this doesn't belong in this PR. Just leaving this here for reference.

I can't do it, if we want to support param values as strings, eg firstName('female') - or I don't know how to do it...

src/index.ts Show resolved Hide resolved
src/modules/name/index.ts Show resolved Hide resolved
src/modules/name/index.ts Outdated Show resolved Hide resolved
src/modules/name/index.ts Outdated Show resolved Hide resolved
src/modules/name/index.ts Outdated Show resolved Hide resolved
src/modules/name/index.ts Outdated Show resolved Hide resolved
src/modules/name/index.ts Outdated Show resolved Hide resolved
src/modules/name/index.ts Outdated Show resolved Hide resolved
src/modules/name/index.ts Outdated Show resolved Hide resolved
src/modules/name/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/modules/name/index.ts Show resolved Hide resolved
src/modules/name/index.ts Show resolved Hide resolved
src/modules/name/index.ts Show resolved Hide resolved
@ejcheng ejcheng added p: 1-normal Nothing urgent c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs labels Jul 21, 2022
@hankucz hankucz requested a review from Shinigami92 July 26, 2022 16:28
@hankucz
Copy link
Contributor Author

hankucz commented Jul 26, 2022

@xDivisionByZerox @ST-DDT @Shinigami92 is this ready for merge? I left some unresolved comments from @Shinigami92 - can you help me with this, as I am not sure if I should tackle them or not as it seems there are some contradictory opinions...

pkuczynski
pkuczynski previously approved these changes Jul 26, 2022
ejcheng
ejcheng previously approved these changes Jul 27, 2022
@xDivisionByZerox xDivisionByZerox added the m: person Something is referring to the person module label Jul 28, 2022
Shinigami92
Shinigami92 previously approved these changes Jul 30, 2022
Co-authored-by: Leyla Jähnig <leyla.jaehnig@gmx.de>
@hankucz hankucz dismissed stale reviews from Shinigami92, ejcheng, ST-DDT, and pkuczynski via 3d693f7 August 1, 2022 13:37
@hankucz
Copy link
Contributor Author

hankucz commented Aug 1, 2022

Suggestions from @xDivisionByZerox are done. Is this ready for merge?

pkuczynski
pkuczynski previously approved these changes Aug 1, 2022
@Shinigami92 Shinigami92 changed the title chore: rename Gender to Sex and use enum instead of string values chore: rename Gender to Sex Aug 1, 2022
@Shinigami92 Shinigami92 enabled auto-merge (squash) August 1, 2022 14:09
@Shinigami92 Shinigami92 merged commit aafab45 into faker-js:main Aug 1, 2022
@hankucz hankucz deleted the gender2sex branch August 3, 2022 10:28
@ST-DDT ST-DDT added this to the v7 - Current Major milestone Aug 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs m: person Something is referring to the person module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Use enum for male/female references
6 participants