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

feat(git): add options.refDate to commitEntry #1512

Merged
merged 2 commits into from
Nov 7, 2022
Merged

feat(git): add options.refDate to commitEntry #1512

merged 2 commits into from
Nov 7, 2022

Conversation

KevinMind
Copy link
Contributor

@KevinMind KevinMind commented Nov 3, 2022 •
edited

closes: #1512

@KevinMind KevinMind requested a review from a team as a code owner November 3, 2022 10:11
@import-brain import-brain requested a review from a team November 3, 2022 10:22
@import-brain import-brain added c: feature Request for new feature m: git Something is referring to the git module labels Nov 3, 2022
@KevinMind
Copy link
Contributor Author

KevinMind commented Nov 3, 2022 •
edited

Seems there is a lagging issue with faker.date.recent() still using slightly varying timestamps when given (1, 'static-date-string'). I'm not totally sure how to work around this. Will have to dig a bit deeper into the date module.

edit: It seems like the recent() method is still using something like current time for the time portion of the returned date.

@KevinMind
Copy link
Contributor Author

Here's an example in a storybook where I tried testing the implementation. The time portion is ticking away.

Screen.Recording.2022-11-03.at.11.27.26.mov

@codecov
Copy link

codecov bot commented Nov 3, 2022 •
edited

Codecov Report

Merging #1512 (fc7dbe9) into next (1bed403) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files 
@@           Coverage Diff           @@
##             next    #1512   +/-   ##
=======================================
  Coverage   99.64%   99.64%           
=======================================
  Files        2213     2213           
  Lines      238633   238637    +4     
  Branches     1014     1014           
=======================================
+ Hits       237779   237783    +4     
  Misses        833      833           
  Partials       21       21
Impacted Files Coverage Δ
src/modules/git/index.ts 100.00% <100.00%> (ø)

@ST-DDT
Copy link
Member

ST-DDT commented Nov 3, 2022

I'm not sure why the story-book has that issue (do you use new Date() or new Date('something-fixed'), but the tests that you have added show, that it is working as expected.

ST-DDT
ST-DDT reviewed Nov 3, 2022
View reviewed changes
test/git.spec.ts Outdated Show resolved Hide resolved
src/modules/git/index.ts Show resolved Hide resolved
@ST-DDT ST-DDT added the s: accepted Accepted feature / Confirmed bug label Nov 3, 2022
@ST-DDT ST-DDT mentioned this pull request Nov 3, 2022
Closed
@KevinMind KevinMind requested a review from ST-DDT November 3, 2022 14:28
@KevinMind
Copy link
Contributor Author

I'm not sure why the story-book has that issue (do you use new Date() or new Date('something-fixed'), but the tests that you have added show, that it is working as expected.

Is there any way to publish canaries for faker? I would be able to commit the case where it is not working to share if so.

ST-DDT
ST-DDT previously approved these changes Nov 3, 2022
View reviewed changes
@ST-DDT
Copy link
Member

ST-DDT commented Nov 3, 2022 •
edited

The code looks good, but it looks like you have to update the test snapshots (and remove the obsolete ones).

@KevinMind KevinMind requested a review from ST-DDT November 4, 2022 12:15
ST-DDT
ST-DDT previously approved these changes Nov 4, 2022
View reviewed changes
@ST-DDT ST-DDT requested a review from a team November 4, 2022 14:22
Shinigami92
Shinigami92 previously approved these changes Nov 4, 2022
View reviewed 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.

Reminder to improve date.recent in

import-brain
import-brain reviewed Nov 4, 2022
View reviewed changes
test/git.spec.ts Outdated Show resolved Hide resolved
test/git.spec.ts Show resolved Hide resolved
@KevinMind KevinMind dismissed stale reviews from Shinigami92 and ST-DDT via 67b1b85 November 6, 2022 10:23
@KevinMind
feat(git): add options.refDate to commitEntry
051a526 
chore: fix PR comments

test: update snapshots

Update test/git.spec.ts

Co-authored-by: Eric Cheng <ericcheng9316@gmail.com>

Update test/git.spec.ts

Co-authored-by: Eric Cheng <ericcheng9316@gmail.com>

feat(git): add options.refDate to commitEntry

chore: fix PR comments

test: update snapshots

Update test/git.spec.ts

Co-authored-by: Eric Cheng <ericcheng9316@gmail.com>

Update test/git.spec.ts

Co-authored-by: Eric Cheng <ericcheng9316@gmail.com>

test: update snapshots
@KevinMind KevinMind requested review from import-brain, Shinigami92 and ST-DDT and removed request for ST-DDT, import-brain and Shinigami92 November 6, 2022 10:34
@KevinMind
Copy link
Contributor Author

@import-brain @ST-DDT @Shinigami92 rebased and updated. If 👍 Then I can merge?

@ST-DDT
Copy link
Member

ST-DDT commented Nov 6, 2022

Since you changed the PR we have to review it again. Please note that only we can press the merge button.

@ST-DDT ST-DDT enabled auto-merge (squash) November 7, 2022 08:40
@ST-DDT ST-DDT merged commit 7c661c6 into faker-js:next Nov 7, 2022
This was referenced Nov 7, 2022
Merged
Merged
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

@Shinigami92 Shinigami92 Shinigami92 approved these changes

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

Assignees

@KevinMind KevinMind

Labels
c: feature Request for new feature m: git Something is referring to the git module s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Faker Roadmap
Status: Done
Milestone
v8.x
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@KevinMind @ST-DDT @Shinigami92 @import-brain