refactor(date)!: birthdate improvements

[ST-DDT]

I found this branch lying around.

• Slightly simplifies the methods implementation/bringing it closer to our standard.
• Tries to improve the error message if min > max
• Adds some additional tests

I'm unsure about the contents of the error message:

• Max age 25 (599616000000) should be greater than or equal to min age 31 (757382400000).
• Max year 1990 (662515200000) should be greater than or equal to min year 2000 (946771200000).

The value in the parenthesis is the epoch timestamp.
Should we remove it?

• Max age 25 should be greater than or equal to min age 31.
• Max year 1990 should be greater than or equal to min year 2000.

What if min or max is undefined?

• Max age 25 should be greater than or equal to min age undefined.
• Max year undefined should be greater than or equal to min year 2000.

We could omit/remove the check if the additional checks from #2719 is merged.

• refactor(date)!: stricter error handling of between #2719

[netlify]

✅ Deploy Preview for fakerjs ready!

Name	Link
🔨 Latest commit	63b60eb
🔍 Latest deploy log	https://app.netlify.com/sites/fakerjs/deploys/660a6f0d59cb790008c2128a
😎 Deploy Preview	https://deploy-preview-2756.fakerjs.dev
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.94%. Comparing base (6191a5d) to head (63b60eb).

Additional details and impacted files

@@            Coverage Diff             @@
##             next    #2756      +/-   ##
==========================================
+ Coverage   99.93%   99.94%   +0.01%     
==========================================
  Files        2960     2960              
  Lines      211617   211726     +109     
  Branches      951      954       +3     
==========================================
+ Hits       211473   211604     +131     
+ Misses        140      118      -22     
  Partials        4        4              

Files	Coverage Δ
src/modules/date/index.ts	100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

[Shinigami92]

If we want these epoch numbers in error message, we might need to add info about that to the message itself

[matthewmayer]

Min and max have defaults (18 and 80) so you should be allowed to omit one or the other?
Under what circumstances would Max age 25 should be greater than or equal to min age undefined. appear?

[ST-DDT]

Min and max have defaults (15 and 80) so you should be allowed to omit one or the other?

As long as you dont violate the other default. Yes.

[matthewmayer]

So I would expect
faker.date.birthdate({max:25}) to work (18-25)
faker.date.birthdate({max:15}) to throw Max age 15 should be greater than or equal to min age 18.

[ST-DDT]

So I would expect faker.date.birthdate({max:25}) to work (18-25) faker.date.birthdate({max:15}) to throw Max age 15 should be greater than or equal to min age 18.

And that would work, if it wasnt for the year mode.
Should I throw two different errors based on mode?

[matthewmayer]

I generally think the more specific the error the better.

[matthewmayer]

Looking at this again in the current docs, it's really weird that the default mode is year and yet the default for min and max are ages.

[ST-DDT]

Looking at this again in the current docs, it's really weird that the default mode is year and yet the default for min and max are ages.

Should we change that as well?

[matthewmayer]

I feel age mode is much more useful. Year mode isn't really much different to between()

[matthewmayer]

Perhaps to help upgrades we should log a warning if you don't specify mode and min/max are say >1000 (because then you surely meant years not ages).

[matthewmayer]

Can we see the updated docs in a netlify preview? The defaults still seem a bit confusing.

[ST-DDT]

Can we see the updated docs in a netlify preview?

Updated

The defaults still seem a bit confusing.

What do you refer to?

[matthewmayer]

Looking at https://deploy-preview-2756.fakerjs.dev/api/date.html#birthdate

• "Returns a random birthdate in the given range of years." doesn't seem ideal. I think we should explain both possible modes here?

• it's not obvious what the default are if you don't pass options object.

• max says "The maximum year to generate a birthdate in." Shouldn't that be "year or age"?

• maybe reorder the parameters mode,min,max,refDate

[ST-DDT]

• "Returns a random birthdate in the given range of years." doesn't seem ideal. I think we should explain both possible modes here?

• it's not obvious what the default are if you don't pass options object.

• max says "The maximum year to generate a birthdate in." Shouldn't that be "year or age"?

I think I might have forgotten to adjust the description after creating the merged signature.

• maybe reorder the parameters mode,min,max,refDate

They are sorted alphabetically (per type in the union), I will not change it in the old generation, if we replace it soon anyway.

[matthewmayer]

Why does the code for mode year need to use the refDate? It seems that should only be needed for age?

[ST-DDT]

Why does the code for mode year need to use the refDate? It seems that should only be needed for age?

Fixed. Ready for review.

[Shinigami92]

• Not sure if we should first merge docs: rewrite api-docs generation using ts-morph #2628

[ST-DDT]

Not sure if we should first merge #2628

IMO that PR doesnt change much regarding this PR. A followup for both displaying/handling multiple signature might though.

I'll likely merge the other PR first, because it is bigger. Unless it is delayed a lot.