Skip to content

refactor(finance)!: simplify account implementation#1992

Merged
ST-DDT merged 7 commits intonextfrom
refactor/finance/simplify-account
Apr 3, 2023
Merged

refactor(finance)!: simplify account implementation#1992
ST-DDT merged 7 commits intonextfrom
refactor/finance/simplify-account

Conversation

@ST-DDT
Copy link
Copy Markdown
Member

@ST-DDT ST-DDT commented Mar 29, 2023 •
edited
Loading

We have an explicit method for generating a numbers with a given length, so lets use that instead.

This PR is a precondition to deprecating helpers.replaceSymbolWithNumber.

@ST-DDT ST-DDT added p: 1-normal Nothing urgent c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs m: finance Something is referring to the finance module labels Mar 29, 2023
@ST-DDT ST-DDT requested review from a team March 29, 2023 19:02
@ST-DDT ST-DDT self-assigned this Mar 29, 2023
ST-DDT
ST-DDT commented Mar 29, 2023
View reviewed changes
Comment thread src/modules/finance/index.ts Outdated
ST-DDT
ST-DDT commented Mar 29, 2023
View reviewed changes
Comment thread src/modules/finance/index.ts Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 29, 2023 •
edited
Loading

Codecov Report

Merging #1992 (b39747a) into next (34b743a) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head b39747a differs from pull request most recent head 3d7f92f. Consider uploading reports for the commit 3d7f92f to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##             next    #1992      +/-   ##
==========================================
+ Coverage   99.61%   99.62%   +0.01%     
==========================================
  Files        2561     2561              
  Lines      243239   243229      -10     
  Branches     1274     1277       +3     
==========================================
+ Hits       242301   242319      +18     
+ Misses        913      885      -28     
  Partials       25       25
Impacted Files Coverage Δ
src/modules/finance/index.ts 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

@ST-DDT ST-DDT mentioned this pull request Mar 29, 2023
Closed
@ST-DDT ST-DDT added s: needs decision Needs team/maintainer decision s: accepted Accepted feature / Confirmed bug breaking change Cannot be merged when next version is not a major release and removed s: needs decision Needs team/maintainer decision labels Mar 29, 2023
@ST-DDT
Copy link
Copy Markdown
Member Author

ST-DDT commented Mar 30, 2023

Team Decision

We will remove the 0 -> 8 mapping.

@ST-DDT ST-DDT changed the title refactor(finance): simplify account implementation refactor(finance)!: simplify account implementation Mar 30, 2023
@ST-DDT ST-DDT requested a review from xDivisionByZerox March 30, 2023 16:37
@Shinigami92
Copy link
Copy Markdown
Member

Shinigami92 commented Mar 31, 2023

What a weird code ...
I'm glad we refactored it

@ST-DDT ST-DDT requested review from a team April 2, 2023 21:06
@ST-DDT ST-DDT enabled auto-merge (squash) April 3, 2023 15:40
@ST-DDT ST-DDT merged commit de078de into next Apr 3, 2023
@ST-DDT ST-DDT deleted the refactor/finance/simplify-account branch April 3, 2023 15:47
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

@xDivisionByZerox xDivisionByZerox xDivisionByZerox approved these changes

Assignees

@ST-DDT ST-DDT

Labels

breaking change Cannot be merged when next version is not a major release c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs m: finance Something is referring to the finance module p: 1-normal Nothing urgent 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.

3 participants

@ST-DDT @Shinigami92 @xDivisionByZerox