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: faker.finance.pin() #695

Merged
merged 13 commits into from
Apr 6, 2022
Merged

feat: faker.finance.pin() #695

merged 13 commits into from
Apr 6, 2022

Conversation

import-brain
Copy link
Member

fixes #472

@import-brain import-brain added c: feature Request for new feature p: 1-normal Nothing urgent labels Mar 27, 2022
@import-brain import-brain requested a review from a team March 27, 2022 14:38
@import-brain import-brain requested a review from a team as a code owner March 27, 2022 14:38
@import-brain import-brain self-assigned this Mar 27, 2022
@codecov
Copy link

codecov bot commented Mar 27, 2022 •
edited

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
test/finance.spec.ts Outdated Show resolved Hide resolved
src/finance.ts Outdated Show resolved Hide resolved
xDivisionByZerox
xDivisionByZerox reviewed Mar 27, 2022
View reviewed changes
src/finance.ts Outdated Show resolved Hide resolved
pkuczynski
pkuczynski reviewed Mar 27, 2022
View reviewed changes
src/finance.ts Outdated Show resolved Hide resolved
@xDivisionByZerox
Copy link
Member

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

@import-brain
Copy link
Member Author

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
Member

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
src/finance.ts Show resolved Hide resolved
test/finance.spec.ts Outdated Show resolved Hide resolved
@import-brain @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
src/finance.ts Outdated Show resolved Hide resolved
@import-brain @pkuczynski
Update src/finance.ts
60b3f94 
Co-authored-by: Piotr Kuczynski <piotr.kuczynski@gmail.com>
@import-brain import-brain 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
src/finance.ts Outdated Show resolved Hide resolved
src/finance.ts Show resolved Hide resolved
import-brain 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
src/finance.ts Outdated Show resolved Hide resolved
@import-brain import-brain 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

@import-brain import-brain

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
@import-brain @xDivisionByZerox @pkuczynski @ST-DDT @Shinigami92