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

fix: remove redundant precision from datatype.datetime options #335

Merged
merged 3 commits into from
Jan 30, 2022

Conversation

pkuczynski
Copy link
Member

@pkuczynski pkuczynski commented Jan 28, 2022

The original DT types for this method looked like this:

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/b5bf4061b0811d2e16dc3c6b4c3c15881b73290f/types/faker/index.d.ts#L106-L107

datetime(max?: number): Date;
datetime(options?: { min?: number | undefined; max?: number | undefined }): Date;

When converting js to ts, it got accidentally typed taking all options of the datatype.number, but using precision here makes no sense, as expected output should be an integer. new Date() can handle floating numbers here

> new Date(1020003333333.1345)
2002-04-28T14:15:33.333Z
> new Date(1020003333333)
2002-04-28T14:15:33.333Z

But I think we should build the interface in a way, which makes more sense.

Additionally I made some changes in the code to prevent precision leaking into datatype.number

@pkuczynski pkuczynski requested a review from a team as a code owner January 28, 2022 16:44
@pkuczynski pkuczynski added the c: bug Something isn't working label Jan 28, 2022
@pkuczynski pkuczynski added this to the v6.0 - Project stability milestone Jan 28, 2022
@pkuczynski pkuczynski added the c: chore PR that doesn't affect the runtime behavior label Jan 28, 2022
@ST-DDT ST-DDT requested a review from a team January 29, 2022 11:41
@Shinigami92
Copy link
Member

#312 needs to be merged first (I know you know it, but just to be clear for other maintainers so they don't accidentally merge this 🙂)

@ST-DDT
Copy link
Member

ST-DDT commented Jan 29, 2022

@Shinigami92 Did you link the wrong PR?
datetime() is in datatype and not in date.

@Shinigami92
Copy link
Member

Shinigami92 commented Jan 29, 2022

@Shinigami92 Did you link the wrong PR? datetime() is in datatype and not in date.

Sorry, I think I loose the overview ...
So I think I need to takle next the datatype tests, will start with that soon

@pkuczynski
Copy link
Member Author

No worries! One step at the time :)

@Shinigami92 Shinigami92 merged commit 9d5a7a2 into faker-js:main Jan 30, 2022
@pkuczynski pkuczynski deleted the datetime-no-precision branch January 30, 2022 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: bug Something isn't working c: chore PR that doesn't affect the runtime behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants