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

refactor(date)!: birthdate improvements #2756

Merged
merged 24 commits into from
Apr 1, 2024
Merged

refactor(date)!: birthdate improvements #2756

merged 24 commits into from
Apr 1, 2024

Conversation

ST-DDT
Copy link
Member

@ST-DDT ST-DDT commented Mar 15, 2024

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.

@ST-DDT ST-DDT added p: 1-normal Nothing urgent c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs m: date Something is referring to the date module labels Mar 15, 2024
@ST-DDT ST-DDT added this to the vAnytime milestone Mar 15, 2024
@ST-DDT ST-DDT requested review from a team March 15, 2024 21:14
@ST-DDT ST-DDT self-assigned this Mar 15, 2024
Copy link

netlify bot commented Mar 15, 2024

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...

QR Code

Use your smartphone camera to open QR code link.

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

Copy link

codecov bot commented Mar 15, 2024

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

Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

test/modules/date.spec.ts Outdated Show resolved Hide resolved
@matthewmayer
Copy link
Contributor

matthewmayer commented Mar 16, 2024

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
Copy link
Member Author

ST-DDT commented Mar 16, 2024

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
Copy link
Contributor

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
Copy link
Member Author

ST-DDT commented Mar 16, 2024

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
Copy link
Contributor

I generally think the more specific the error the better.

@matthewmayer
Copy link
Contributor

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
Copy link
Member Author

ST-DDT commented Mar 16, 2024

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
Copy link
Contributor

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

@ST-DDT ST-DDT added the breaking change Cannot be merged when next version is not a major release label Mar 16, 2024
@ST-DDT ST-DDT changed the title refactor(date): birthday improvements refactor(date)!: birthday improvements Mar 16, 2024
@ST-DDT ST-DDT changed the title refactor(date)!: birthday improvements refactor(date)!: birthdate improvements Mar 16, 2024
@ST-DDT ST-DDT requested review from matthewmayer, a team and Shinigami92 March 16, 2024 17:14
@matthewmayer
Copy link
Contributor

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
Copy link
Contributor

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

@ST-DDT
Copy link
Member Author

ST-DDT commented Mar 28, 2024

Can we see the updated docs in a netlify preview?

Updated

The defaults still seem a bit confusing.

What do you refer to?

@matthewmayer
Copy link
Contributor

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
Copy link
Member Author

ST-DDT commented Mar 28, 2024

  • "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.

@ST-DDT ST-DDT dismissed stale reviews from import-brain and Shinigami92 via 3fd134d March 28, 2024 18:29
@matthewmayer
Copy link
Contributor

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

@ST-DDT
Copy link
Member Author

ST-DDT commented Mar 29, 2024

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.

@ST-DDT ST-DDT requested review from Shinigami92 and a team March 29, 2024 23:34
src/modules/date/index.ts Show resolved Hide resolved
src/modules/date/index.ts Outdated Show resolved Hide resolved
@ST-DDT ST-DDT requested review from matthewmayer and a team March 30, 2024 17:06
Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ST-DDT
Copy link
Member Author

ST-DDT commented Mar 31, 2024

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.

@ST-DDT ST-DDT enabled auto-merge (squash) April 1, 2024 08:27
@ST-DDT ST-DDT merged commit b498d1f into next Apr 1, 2024
20 checks passed
@Shinigami92 Shinigami92 deleted the test/date/birthday branch April 1, 2024 08:51
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: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs m: date Something is referring to the date module p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants