-
-
Notifications
You must be signed in to change notification settings - Fork 953
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
refactor(person): refine usage of PersonEntryDefinitions #3259
base: next
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for fakerjs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## next #3259 +/- ##
=======================================
Coverage 99.97% 99.97%
=======================================
Files 2811 2811
Lines 217414 217437 +23
Branches 949 958 +9
=======================================
+ Hits 217359 217383 +24
+ Misses 55 54 -1
🚀 New features to boost your workflow:
|
I understand wanting to keep this PR small but I think it's difficult to decide if this is the right approach without reviewing what locale files will need to be updated too. To clarify with this approach would we require the "generic", "female" and "male" options never have any items in common? |
Team Decision We will change the sexType method to return all SexType values. ST-DDT: Will create a stacked PR for the locale changes.
|
@xDivisionByZerox Currently, the person files aren't sorted, so this by itself causes a massive diff. (Maybe start with only sorting first, because the cleanup PR will use the list for filtering: See #3266 (comment)) |
Please go ahead. I'm not available during the weekend, sadly. |
Updated the distribution to match the result from our team meeting. Ready for review. |
Co-authored-by: Eric Cheng <ericcheng9316@gmail.com>
Read for review |
return ( | ||
generic ?? | ||
// The last statement should never happen at run time. At this point in time, | ||
// the entry will satisfy at least (generic || (female && male)). | ||
// TS is not able to infer the type correctly. | ||
[] | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "catch" case here, that basically duplicates the generic part, could be prevented by using a switch with an explicit handle for 'female' | 'male'
and an implicit one (via the default case) for generic and the unhandled scenario when binary == null
. I'd be in favor of structuring like this. Other ideas?
Fixes #3058
generic
prefixes #3058Next steps: #3266
Meeting-Notes: 2024-11-07
This PR tries to lay the foundation of what is considered generic and what is specifically sexed:
Dr.
)Mrs.
/Mr.
)It also changes the way how definitions are selected:
Please check whether the definitions/jsdoc are clear regarding their intentions and if you consider this definition to be correct.
We will clean up the locale data in separate PRs as this might affect lots of files/locales.