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(fake): move to helpers #1161

Merged
merged 12 commits into from
Jul 30, 2022
Merged

refactor(fake): move to helpers #1161

merged 12 commits into from
Jul 30, 2022

Conversation

xDivisionByZerox
Copy link
Member

This PR moves the fake() method from the Fake module into the Helpers module. The reason is, that there is no reason for it being a standalone module when it is clearly a utility method.

This was also discussed in #805 (direct link).

@xDivisionByZerox xDivisionByZerox added the c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs label Jul 18, 2022
@xDivisionByZerox xDivisionByZerox added this to the v7 - Current Major milestone Jul 18, 2022
@xDivisionByZerox xDivisionByZerox requested a review from a team July 18, 2022 15:47
@xDivisionByZerox xDivisionByZerox requested a review from a team as a code owner July 18, 2022 15:47
@xDivisionByZerox xDivisionByZerox self-assigned this Jul 18, 2022
@codecov
Copy link

codecov bot commented Jul 18, 2022 •
edited
Loading

Codecov Report

Merging #1161 (eda6569) into main (09df73a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1161   +/-   ##
=======================================
  Coverage   99.62%   99.62%           
=======================================
  Files        2152     2152           
  Lines      236234   236291   +57     
  Branches      973      976    +3     
=======================================
+ Hits       235344   235411   +67     
+ Misses        869      859   -10     
  Partials       21       21
Impacted Files Coverage Δ
src/modules/address/index.ts 99.80% <100.00%> (ø)
src/modules/company/index.ts 100.00% <100.00%> (ø)
src/modules/fake/index.ts 100.00% <100.00%> (ø)
src/modules/helpers/index.ts 98.29% <100.00%> (+0.48%) ⬆️
src/modules/finance/index.ts 100.00% <0.00%> (+0.68%) ⬆️
src/modules/internet/user-agent.ts 83.59% <0.00%> (+1.85%) ⬆️

pkuczynski
pkuczynski previously approved these changes Jul 18, 2022
View reviewed changes
Shinigami92
Shinigami92 reviewed Jul 18, 2022
View reviewed changes
Copy link
Member

@Shinigami92 Shinigami92 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Also just add a deprecation JSDoc Tag to the module + its constructor

Did you also update references in docs and readme?

@xDivisionByZerox
Copy link
Member Author

Did you also update references in docs and readme?

Nah, I didn't check for that. Good catch. I really have to stop ONLY using the find all references feature^^

@xDivisionByZerox
Copy link
Member Author

xDivisionByZerox commented Jul 18, 2022 •
edited by Shinigami92
Loading

Also just add a deprecation JSDoc Tag to the module + its constructor

This is not possible since this would log a warning on Faker instance creation, which violates one of our test suites. I mean I can still add a JSDoc with a @deprecated parameter, but no deprecated function call in constructor.

@xDivisionByZerox xDivisionByZerox requested a review from a team July 18, 2022 21:14
@Shinigami92
Copy link
Member

Shinigami92 commented Jul 19, 2022 •
edited
Loading

Also just add a deprecation JSDoc Tag to the module + its constructor

This is not possible since this would log a warning on Faker instance creation, which violates one of our test suites. I mean I can still add a JSDoc with a @deprecated parameter, but no deprecated function call in constructor.

Good catch, that's true

Shinigami92
Shinigami92 previously approved these changes Jul 19, 2022
View reviewed changes
ST-DDT
ST-DDT previously approved these changes Jul 19, 2022
View reviewed changes
@xDivisionByZerox xDivisionByZerox added p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug labels Jul 19, 2022
@ST-DDT ST-DDT requested review from a team July 20, 2022 09:32
pkuczynski
pkuczynski previously approved these changes Jul 20, 2022
View reviewed changes
import-brain
import-brain previously approved these changes Jul 22, 2022
View reviewed changes
@xDivisionByZerox
Copy link
Member Author

We had 4 individual approvals and this is non blocking. I think we can merge this.

@ST-DDT ST-DDT enabled auto-merge (squash) July 23, 2022 04:22
@ST-DDT
Copy link
Member

ST-DDT commented Jul 23, 2022

The tests seem to fail.

@xDivisionByZerox
Copy link
Member Author

That's odd, but I'll have a look.

@xDivisionByZerox
Copy link
Member Author

The tests seem to fail.

It was a missing test with the new test factory. @ST-DDT can you have a look if I've done this the right way?

Shinigami92
Shinigami92 approved these changes Jul 30, 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.

The implementation is approved, but I think this PR needs a rebase as it is older than the test overhaul, is it?

@Shinigami92 Shinigami92 added needs rebase There is a merge conflict and removed s: needs decision Needs team/maintainer decision s: on hold Blocked by something or frozen to avoid conflicts labels Jul 30, 2022
@ST-DDT ST-DDT removed the needs rebase There is a merge conflict label Jul 30, 2022
@ST-DDT ST-DDT enabled auto-merge (squash) July 30, 2022 10:54
@ST-DDT
Copy link
Member

ST-DDT commented Jul 30, 2022

but I think this PR needs a rebase as it is older than the test overhaul, is it?

It has been updated since.

@ST-DDT ST-DDT merged commit fc48155 into main Jul 30, 2022
@ST-DDT ST-DDT deleted the refactor/fake/move-to-helpers branch July 30, 2022 10:57
@leosuncin leosuncin mentioned this pull request Aug 18, 2022
Merged
@xDivisionByZerox xDivisionByZerox added the deprecation A deprecation was made in the PR label Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Shinigami92 Shinigami92 Shinigami92 approved these changes

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

@import-brain import-brain Awaiting requested review from import-brain

@pkuczynski pkuczynski Awaiting requested review from pkuczynski

Assignees

@xDivisionByZerox xDivisionByZerox

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: helpers Something is referring to the helpers module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Faker Roadmap
Status: Done
Milestone
v7 - Current Major
Development

Successfully merging this pull request may close these issues.
5 participants
@xDivisionByZerox @Shinigami92 @ST-DDT @pkuczynski @import-brain