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

Change file structure for gendered lists (names, prefixes etc) #1677

Closed
matthewmayer opened this issue Dec 21, 2022 · 12 comments · Fixed by #2938
Closed

Change file structure for gendered lists (names, prefixes etc) #1677

matthewmayer opened this issue Dec 21, 2022 · 12 comments · Fixed by #2938
Assignees
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 m: person Something is referring to the person module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Milestone

Comments

@matthewmayer
Copy link
Contributor

Originally posted by @ST-DDT in #547 (comment)

Change data-structure:

name.female_first_name ->person.first_name.female

Example:

type Gendered = {
  female: string[]
  male: string[]
  generic: string[]
};
@matthewmayer
Copy link
Contributor Author

matthewmayer commented Dec 21, 2022

Also noting that the first names for the following languages are not split by gender currently, someone with better knowledge could check these:

de_CH
es_MX
ka_GE
ko
ne
zh_TW

@ST-DDT ST-DDT added p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs c: locale Permutes locale definitions m: person Something is referring to the person module labels Dec 22, 2022
@ST-DDT ST-DDT added this to the v8.0 - Module Re-Shuffling milestone Dec 22, 2022
@ST-DDT ST-DDT moved this to Todo in Faker Roadmap Dec 22, 2022
@Shinigami92
Copy link
Member

Also noting that the first names for the following languages are not split by gender currently, someone with better knowledge could check these:

de_CH
es_MX
ge
ko
ne
zh_TW

For now I would say put them in person.first_name.generic (?) or person.first_name.index (?)
So with this change they are not anymore in the file but the folder

@ST-DDT
Copy link
Member

ST-DDT commented Dec 24, 2022

I prefer generic over index. (Could you explain why you suggested index? I'm unable to guess your reasons/thoughts here.)

@matthewmayer
Copy link
Contributor Author

It would be great to make a list of contributors (even if they are not that regular) who know various languages so we can ask their help for locale specific issues. So if say there's an issue with Korean we can loop them in.

@Shinigami92
Copy link
Member

I prefer generic over index. (Could you explain why you suggested index? I'm unable to guess your reasons/thoughts here.)

No I cannot => use generic

@matthewmayer
Copy link
Contributor Author

@Stichoza it looks like you originally added the data for Georgian, i wondered if you might be able to help us split the https://github.com/faker-js/faker/blob/next/src/locales/ge/person/first_name.ts file into male and female? This will help the ge locale work correctly.

https://github.com/faker-js/faker/commits?author=Stichoza

@matthewmayer
Copy link
Contributor Author

I think this will also require changes to faker.helpers.fake to get it to work with patterns like {{person.first_name}}?

@ST-DDT
Copy link
Member

ST-DDT commented Apr 7, 2023

No, it doesnt or at least we wont implement it that way. Instead we will change the patterns accordingly.e.g. person.first_name.generic/...

But thanks anyway, this is a good reminder for the actual implementation since we didnt write that anywhere.

@matthewmayer
Copy link
Contributor Author

i think there might be some logic in deferring this until 8.1. It's a fairly large change which will affect a lot of patterns and tests, but shouldnt have breaking changes (if implemented sensibly) and stuffing too much into 8.0 will delay and delay it.

@ST-DDT ST-DDT added the s: needs decision Needs team/maintainer decision label Apr 8, 2023
@ST-DDT
Copy link
Member

ST-DDT commented Apr 13, 2023

Team Decision

We don't need this for v8.0.
(We will likely immediately start with v9 after v8.0)

@ST-DDT ST-DDT removed the s: needs decision Needs team/maintainer decision label Apr 13, 2023
@ST-DDT ST-DDT removed this from the v8.0 - Module Re-Shuffling milestone Apr 13, 2023
@matthewmayer
Copy link
Contributor Author

I don't feel this is essential for V9

@xDivisionByZerox
Copy link
Member

I don't feel this is essential for V9

It's not essential, but we kicked this issue since pre v7. Since this would be breaking, we wanted to still include this in the next major.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 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: Todo
Development

Successfully merging a pull request may close this issue.

4 participants