faker icon indicating copy to clipboard operation
faker copied to clipboard

refactor(person): refine usage of PersonEntryDefinitions

Open ST-DDT opened this issue 1 year ago β€’ 9 comments

Fixes #3058

  • #3058

Next steps: #3266

  • #3266

Meeting-Notes: 2024-11-07


This PR tries to lay the foundation of what is considered generic and what is specifically sexed:

  • Generic: Values that can be attributed to/used by either of male or female persons. (e.g. Dr.)
  • Female/Male: Values that can be attributed to specifically female/male (e.g. Mrs./Mr.)

It also changes the way how definitions are selected:

  • If generic is requested: Then only values that are specifically generic are returned, unless there are no such values.
  • If female/male is requested: Then the method will mostly (80%) return female/male values with some (20%) generic values sprinkled in. Since generic can explicitly be both, the generic elements don't cause any issues.

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.

ST-DDT avatar Nov 12 '24 21:11 ST-DDT

Deploy Preview for fakerjs ready!

Name Link
Latest commit 612d6b4c222618ca713f238485a717c58184a40e
Latest deploy log https://app.netlify.com/sites/fakerjs/deploys/67c9df396944850008d09824
Deploy Preview https://deploy-preview-3259.fakerjs.dev
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] avatar Nov 12 '24 21:11 netlify[bot]

Codecov Report

Attention: Patch coverage is 94.87179% with 2 lines in your changes missing coverage. Please review.

Project coverage is 99.97%. Comparing base (62486af) to head (612d6b4).

Files with missing lines Patch % Lines
src/modules/person/index.ts 94.11% 2 Missing :warning:
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     
Files with missing lines Coverage Ξ”
src/definitions/person.ts 100.00% <ΓΈ> (ΓΈ)
src/modules/image/index.ts 100.00% <100.00%> (ΓΈ)
src/modules/person/index.ts 96.66% <94.11%> (-0.28%) :arrow_down:

... and 1 file with indirect coverage changes

πŸš€ New features to boost your workflow:
  • ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • πŸ“¦ JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Nov 12 '24 21:11 codecov[bot]

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?

matthewmayer avatar Nov 14 '24 14:11 matthewmayer

Team Decision

We will change the sexType method to return all SexType values. A parameter includeGeneric: boolean = false can be used to control the inclusion of generic in the results. We will check in v10 whether we want to change the default for includeGeneric based on code usage?

ST-DDT: Will create a stacked PR for the locale changes.

  • Things in both female and male -> generic
  • Things in generic -> remove from female and male
  • Without manual changes
  • The stacked PR might be later split in smaller chunks to help with reviewability

ST-DDT avatar Nov 14 '24 21:11 ST-DDT

ST-DDT: Will create a stacked PR for the locale changes.

[...]

  • Without manual changes
  • The stacked PR might be later split in smaller chunks to help with reviewability

@xDivisionByZerox Currently, the person files aren't sorted, so this by itself causes a massive diff. Should we sort them first? Would you like to create a PR for that or should I?

(Maybe start with only sorting first, because the cleanup PR will use the list for filtering: See https://github.com/faker-js/faker/pull/3266#issuecomment-2478817148)

ST-DDT avatar Nov 15 '24 12:11 ST-DDT

@xDivisionByZerox Currently, the person files aren't sorted, so this by itself causes a massive diff. Should we sort them first? Would you like to create a PR for that or should I?

Please go ahead. I'm not available during the weekend, sadly.

xDivisionByZerox avatar Nov 15 '24 13:11 xDivisionByZerox

Done: https://github.com/faker-js/faker/pull/3269

  • https://github.com/faker-js/faker/pull/3269

ST-DDT avatar Nov 16 '24 11:11 ST-DDT

Updated the distribution to match the result from our team meeting.

  • https://github.com/faker-js/faker/pull/3266#issuecomment-2555258729

Ready for review.

ST-DDT avatar Dec 24 '24 14:12 ST-DDT

Read for review

ST-DDT avatar Feb 12 '25 20:02 ST-DDT