Skip to content

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

Merged
ST-DDT merged 2 commits into
faker-js:nextfrom
KevinMind:next
Nov 7, 2022
Merged

feat(git): add options.refDate to commitEntry#1512
ST-DDT merged 2 commits into
faker-js:nextfrom
KevinMind:next

Conversation

@KevinMind

@KevinMind KevinMind commented Nov 3, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

closes: #1512
@KevinMind KevinMind requested a review from a team as a code owner November 3, 2022 10:11
@ejcheng ejcheng requested a review from a team November 3, 2022 10:22
@ejcheng ejcheng added c: feature Request for new feature m: git Something is referring to the git module labels Nov 3, 2022
@KevinMind

KevinMind commented Nov 3, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

KevinMind commented Nov 3, 2022

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

codecov Bot commented Nov 3, 2022 •
edited
Loading

Copy link
Copy Markdown

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

ST-DDT commented Nov 3, 2022

Copy link
Copy Markdown
Member

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
Comment thread test/git.spec.ts Outdated
Comment thread src/modules/git/index.ts
@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

KevinMind commented Nov 3, 2022

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

ST-DDT commented Nov 3, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

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

@Shinigami92 Shinigami92 left a comment

Copy link
Copy Markdown
Member

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
ejcheng
ejcheng reviewed Nov 4, 2022
View reviewed changes
Comment thread test/git.spec.ts Outdated
Comment thread test/git.spec.ts
@KevinMind KevinMind dismissed stale reviews from Shinigami92 and ST-DDT via 67b1b85 November 6, 2022 10:23
@KevinMind KevinMind force-pushed the next branch 3 times, most recently from 6e92008 to 6c6899e Compare November 6, 2022 10:33
@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 ST-DDT, Shinigami92 and ejcheng and removed request for ST-DDT, Shinigami92 and ejcheng November 6, 2022 10:34
@KevinMind

KevinMind commented Nov 6, 2022

Copy link
Copy Markdown
Contributor Author

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

ST-DDT commented Nov 6, 2022

Copy link
Copy Markdown
Member

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

Labels

c: feature Request for new feature m: git Something is referring to the git module s: accepted Accepted feature / Confirmed bug

4 participants

@KevinMind @ST-DDT @Shinigami92 @ejcheng