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.

Sign up for GitHub

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

Jump to bottom

refactor(datatype): deprecate datetime #2053

Merged
merged 22 commits into from
Apr 22, 2023
Merged

refactor(datatype): deprecate datetime #2053

merged 22 commits into from
Apr 22, 2023

Conversation

Shinigami92
Copy link
Member

@Shinigami92 Shinigami92 commented Apr 15, 2023 •
edited by xDivisionByZerox

Deprecate datatype.datetime in favor of date.between

@Shinigami92 Shinigami92 added 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 Apr 15, 2023
@Shinigami92 Shinigami92 requested a review from a team as a code owner April 15, 2023 16:26
@Shinigami92 Shinigami92 self-assigned this Apr 15, 2023
@codecov
Copy link

codecov bot commented Apr 15, 2023 •
edited

Codecov Report

Merging #2053 (fc731cc) into next (a49aa0d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files 
@@           Coverage Diff           @@
##             next    #2053   +/-   ##
=======================================
  Coverage   99.59%   99.59%           
=======================================
  Files        2538     2538           
  Lines      242645   242656   +11     
  Branches     1301     1302    +1     
=======================================
+ Hits       241663   241684   +21     
+ Misses        955      945   -10     
  Partials       27       27
Impacted Files Coverage Δ
src/modules/datatype/index.ts 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

ST-DDT
ST-DDT reviewed Apr 15, 2023
View reviewed changes
Copy link
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

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

Please include this in the upgrading guide.

src/modules/date/index.ts Outdated Show resolved Hide resolved
src/modules/date/index.ts Outdated Show resolved Hide resolved
src/modules/date/index.ts Outdated Show resolved Hide resolved
src/modules/datatype/index.ts Outdated Show resolved Hide resolved
@Shinigami92 Shinigami92 requested a review from a team April 16, 2023 10:29
@ST-DDT
Copy link
Member

ST-DDT commented Apr 20, 2023

Team Decision

We won't move it over because it behaves the same as faker.date.between, but we will adjust the deprecation message accordingly.

@Shinigami92 Shinigami92 marked this pull request as draft April 20, 2023 17:34
@Shinigami92 Shinigami92 added the deprecation A deprecation was made in the PR label Apr 20, 2023
@Shinigami92 Shinigami92 marked this pull request as ready for review April 20, 2023 18:03
ST-DDT
ST-DDT previously approved these changes Apr 20, 2023
View reviewed changes
docs/guide/upgrading.md Outdated Show resolved Hide resolved
@ST-DDT ST-DDT added the s: accepted Accepted feature / Confirmed bug label Apr 20, 2023
@ST-DDT ST-DDT requested a review from a team April 20, 2023 18:27
@ST-DDT
Copy link
Member

ST-DDT commented Apr 20, 2023

The PR title + labels needs updating.

@xDivisionByZerox xDivisionByZerox changed the title refactor(date): move datetime to date module refactor(datatype): deprecate datetime Apr 20, 2023
@xDivisionByZerox xDivisionByZerox added m: datatype Something is referring to the datatype module and removed m: date Something is referring to the date module labels Apr 20, 2023
@ST-DDT ST-DDT requested review from a team April 21, 2023 18:12
@ST-DDT ST-DDT requested a review from a team April 22, 2023 11:21
@Shinigami92 Shinigami92 merged commit 8fc5261 into next Apr 22, 2023
17 checks passed
@Shinigami92 Shinigami92 deleted the move-datetime-to-date-module branch April 22, 2023 19:03
@Shinigami92
Copy link
Member Author

@matthewmayer @ST-DDT due to the requested reviews from your side to use between instead of its own function, it slipped into the next branch that we dont have a method anymore that just provides a random date but without arguments/parameters
We need to fix that before releasing v8.0

@Shinigami92 Shinigami92 mentioned this pull request Apr 25, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ST-DDT ST-DDT ST-DDT approved these changes

@import-brain import-brain import-brain approved these changes

@matthewmayer matthewmayer Awaiting requested review from matthewmayer

@xDivisionByZerox xDivisionByZerox Awaiting requested review from xDivisionByZerox

Assignees

@Shinigami92 Shinigami92

Labels
c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs deprecation A deprecation was made in the PR m: datatype Something is referring to the datatype module s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Faker Roadmap
Status: Done
Milestone
v8.0 - Module Re-Shuffling
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Shinigami92 @ST-DDT @matthewmayer @import-brain @xDivisionByZerox