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

Use enum for male/female references #352

Closed
griest024 opened this issue Jan 29, 2022 · 10 comments · Fixed by #1163
Closed

Use enum for male/female references #352

griest024 opened this issue Jan 29, 2022 · 10 comments · Fixed by #1163
Labels
c: chore PR that doesn't affect the runtime behavior m: person Something is referring to the person module s: needs decision Needs team/maintainer decision

Comments

@griest024
Copy link
Contributor

griest024 commented Jan 29, 2022

Clear and concise description of the problem

References to male and female are done with hardcoded strings and numbers.

faker/src/name.ts

Lines 29 to 35 in 44628ec

if (typeof gender === 'string') {
if (gender.toLowerCase() === 'male') {
gender = 0;
} else if (gender.toLowerCase() === 'female') {
gender = 1;
}
}
.

Suggested solution

These should be replaced with enum values:

enum Sex {
  MALE = 'male',
  FEMALE = 'female',
  INTERSEX = 'intersex'
}

Alternative

No response

Additional context

As an aside, gender is an inaccurate name for male and female. The correct term is sex.

@griest024 griest024 added the s: pending triage Pending Triage label Jan 29, 2022
@Shinigami92
Copy link
Member

I would suggest to use type literals like type Sex = 'male' | 'female'

Beside that, I would highly like to have something like type Sex = 'male' | 'female' | 'binary' in the future

@griest024
Copy link
Contributor Author

I would suggest to use type literals like type Sex = 'male' | 'female'

Beside that, I would highly like to have something like type Sex = 'male' | 'female' | 'binary' in the future

I much prefer enums; simple types can't be used as runtime values and we can't use it to replace hardcoded values.

@Shinigami92
Copy link
Member

I like to provide enums, but not to force the usage
So we could to

export enum Sex {
  MALE = 'male',
  FEMALE = 'female',
  INTERSEX = 'intersex'
}

export type SexType = 'male' | 'female' | 'intersex' | number

firstName(gender?: SexType): string { /*...*/ }

@griest024
Copy link
Contributor Author

Yes as to the external usage we should accept more than just the enum. But for the internal usage I want to see case Sex.MALE: instead of case 0:.

@ST-DDT
Copy link
Member

ST-DDT commented Jan 29, 2022

Please no magic numbers. Or what is your intention?

EIDT: Backwards compatibility.

@Shinigami92
Copy link
Member

TS Playground

@import-brain import-brain added c: chore PR that doesn't affect the runtime behavior and removed s: pending triage Pending Triage labels Feb 2, 2022
@import-brain import-brain moved this to In Progress in Faker Roadmap Feb 15, 2022
@import-brain import-brain added the s: needs decision Needs team/maintainer decision label Apr 12, 2022
@pkuczynski
Copy link
Member

What would intersex or binary mean in this case? https://en.wikipedia.org/wiki/Sex does not list those terms...

@ST-DDT
Copy link
Member

ST-DDT commented Jul 3, 2022

IMO this issue is already implemented.

export enum Gender {
female = 'female',
male = 'male',
}

@pkuczynski
Copy link
Member

I was only scanning the issues. If it's already implemented, we should close it. @griest024 wdyt?

@pkuczynski
Copy link
Member

Btw. there is #353 suggesting renaming gender to sex...

@xDivisionByZerox xDivisionByZerox added the m: person Something is referring to the person module label Jul 30, 2022
Repository owner moved this from In Progress to Done in Faker Roadmap Aug 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: chore PR that doesn't affect the runtime behavior m: person Something is referring to the person module s: needs decision Needs team/maintainer decision
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

6 participants