Skip to content

feat: faker.finance.pin()#695

Merged
Shinigami92 merged 13 commits intomainfrom
feat_pin
Apr 6, 2022
Merged

feat: faker.finance.pin()#695
Shinigami92 merged 13 commits intomainfrom
feat_pin

Conversation

@ejcheng
Copy link
Copy Markdown
Member

@ejcheng ejcheng commented Mar 27, 2022

fixes #472

@ejcheng ejcheng added c: feature Request for new feature p: 1-normal Nothing urgent labels Mar 27, 2022
@ejcheng ejcheng added this to the v6.2 - New small features milestone Mar 27, 2022
@ejcheng ejcheng requested a review from a team March 27, 2022 14:38
@ejcheng ejcheng requested a review from a team as a code owner March 27, 2022 14:38
@ejcheng ejcheng self-assigned this Mar 27, 2022
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 27, 2022 •
edited
Loading

Codecov Report

Merging #695 (a8abe45) into main (5beac4b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head a8abe45 differs from pull request most recent head 421299e. Consider uploading reports for the commit 421299e to get more accurate results

@@           Coverage Diff           @@
##             main     #695   +/-   ##
=======================================
  Coverage   99.34%   99.34%           
=======================================
  Files        1921     1921           
  Lines      176656   176673   +17     
  Branches      905      909    +4     
=======================================
+ Hits       175503   175520   +17     
  Misses       1097     1097           
  Partials       56       56
Impacted Files Coverage Δ
src/finance.ts 99.33% <100.00%> (+0.02%) ⬆️

pkuczynski
pkuczynski requested changes Mar 27, 2022
View reviewed changes
Comment thread test/finance.spec.ts Outdated
Comment thread src/finance.ts Outdated
xDivisionByZerox
xDivisionByZerox reviewed Mar 27, 2022
View reviewed changes
Comment thread src/finance.ts Outdated
pkuczynski
pkuczynski reviewed Mar 27, 2022
View reviewed changes
Comment thread src/finance.ts Outdated
@xDivisionByZerox
Copy link
Copy Markdown
Member

xDivisionByZerox commented Mar 27, 2022

What would be the expected behavior when a negative length is requested?

@ejcheng
Copy link
Copy Markdown
Member Author

ejcheng commented Mar 27, 2022

What would be the expected behavior when a negative length is requested?

I didn't consider that scenario. Do you think it should throw an error?

Shinigami92
Shinigami92 previously approved these changes Mar 27, 2022
View reviewed changes
@pkuczynski
Copy link
Copy Markdown
Member

pkuczynski commented Mar 27, 2022

What would be the expected behavior when a negative length is requested?

I didn't consider that scenario. Do you think it should throw an error?

I personally think it should throw when < 1...

pkuczynski
pkuczynski requested changes Mar 27, 2022
View reviewed changes
Comment thread src/finance.ts
Comment thread test/finance.spec.ts Outdated
@ejcheng @pkuczynski
fix: make test name more clear
42248c4 
Co-authored-by: Piotr Kuczynski <piotr.kuczynski@gmail.com>
pkuczynski
pkuczynski reviewed Mar 27, 2022
View reviewed changes
Comment thread src/finance.ts Outdated
@ejcheng ejcheng dismissed stale reviews from pkuczynski and xDivisionByZerox via 60b3f94 March 27, 2022 18:19
@ejcheng @pkuczynski
Update src/finance.ts
60b3f94 
Co-authored-by: Piotr Kuczynski <piotr.kuczynski@gmail.com>
@ejcheng ejcheng requested review from a team and removed request for Shinigami92 March 27, 2022 18:19
ST-DDT
ST-DDT reviewed Mar 27, 2022
View reviewed changes
Comment thread src/finance.ts Outdated
Comment thread src/finance.ts
ejcheng and others added 2 commits March 27, 2022 14:35
ST-DDT
ST-DDT previously approved these changes Mar 27, 2022
View reviewed changes
pkuczynski
pkuczynski previously approved these changes Mar 27, 2022
View reviewed changes
Shinigami92
Shinigami92 requested changes Apr 5, 2022
View reviewed changes
Comment thread src/finance.ts Outdated
@ejcheng ejcheng dismissed stale reviews from pkuczynski and ST-DDT via a8abe45 April 6, 2022 00:59
@Shinigami92 Shinigami92 enabled auto-merge (squash) April 6, 2022 07:51
@Shinigami92 Shinigami92 merged commit 20f33e6 into main Apr 6, 2022
@Shinigami92 Shinigami92 deleted the feat_pin branch April 6, 2022 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pkuczynski pkuczynski pkuczynski approved these changes

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

@Shinigami92 Shinigami92 Shinigami92 approved these changes

@xDivisionByZerox xDivisionByZerox Awaiting requested review from xDivisionByZerox

Assignees

@ejcheng ejcheng

Labels

c: feature Request for new feature p: 1-normal Nothing urgent

Projects

None yet

Milestone

v6.2 - New small features

Development

Successfully merging this pull request may close these issues.

PIN, account number

5 participants

@ejcheng @xDivisionByZerox @pkuczynski @ST-DDT @Shinigami92