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: reimplement datatype.bigInt #791

Merged
merged 8 commits into from
May 23, 2022
Merged

feat: reimplement datatype.bigInt #791

merged 8 commits into from
May 23, 2022

Conversation

Shinigami92
Copy link
Member

@Shinigami92 Shinigami92 commented Apr 6, 2022 •
edited
Loading

Depends on #797

@Shinigami92 Shinigami92 added c: feature Request for new feature p: 1-normal Nothing urgent labels Apr 6, 2022
@Shinigami92 Shinigami92 added this to the v6.2 - New small features milestone Apr 6, 2022
@Shinigami92 Shinigami92 self-assigned this Apr 6, 2022
@Shinigami92 Shinigami92 linked an issue Apr 6, 2022 that may be closed by this pull request
datatype.bigint takes value and gives it just back #348
Closed
@Shinigami92 Shinigami92 added the needs test More tests are needed label Apr 6, 2022
pkuczynski
pkuczynski requested changes Apr 6, 2022
View reviewed changes
src/datatype.ts Outdated Show resolved Hide resolved
src/datatype.ts Outdated Show resolved Hide resolved
src/datatype.ts Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Apr 6, 2022 •
edited
Loading

Codecov Report

Merging #791 (a846ac9) into main (73db3a7) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #791   +/-   ##
=======================================
  Coverage   99.67%   99.67%           
=======================================
  Files        2110     2110           
  Lines      225693   225732   +39     
  Branches      971      979    +8     
=======================================
+ Hits       224950   224992   +42     
+ Misses        723      720    -3     
  Partials       20       20
Impacted Files Coverage Δ
src/modules/datatype/index.ts 100.00% <100.00%> (ø)
src/modules/finance/index.ts 100.00% <0.00%> (+0.68%) ⬆️

@Shinigami92 Shinigami92 added s: accepted Accepted feature / Confirmed bug and removed needs test More tests are needed labels Apr 6, 2022
@Shinigami92 Shinigami92 requested review from pkuczynski, ST-DDT and a team April 6, 2022 22:48
ST-DDT
ST-DDT requested changes Apr 7, 2022
View reviewed changes
src/datatype.ts Outdated Show resolved Hide resolved
src/datatype.ts Outdated Show resolved Hide resolved
test/datatype.spec.ts Show resolved Hide resolved
test/datatype.spec.ts Outdated Show resolved Hide resolved
@ST-DDT
Copy link
Member

ST-DDT commented Apr 7, 2022 •
edited
Loading

Please also add a test, that uses min:0, max: 1000000000000 (it is important that these are 1 and then only zeros, as these are harder to "find" for some implementations (e.g. loop based ones).

00-99 => 100% (100/100) success rate
00-10 => ~10% (11/100) success rate

@Shinigami92 Shinigami92 requested a review from ST-DDT April 7, 2022 11:12
Shinigami92
Shinigami92 commented Apr 7, 2022
View reviewed changes
src/datatype.ts Outdated Show resolved Hide resolved
@Shinigami92 Shinigami92 mentioned this pull request Apr 7, 2022
Merged
@Shinigami92 Shinigami92 added the s: on hold Blocked by something or frozen to avoid conflicts label Apr 8, 2022
@Shinigami92
Copy link
Member Author

Awaiting other PRs like #797 to be done

@Shinigami92 Shinigami92 mentioned this pull request Apr 8, 2022
Merged
@Shinigami92 Shinigami92 force-pushed the 348-improve-datatype-bigint branch from 8f04b48 to a77f218 Compare April 8, 2022 10:10
@Shinigami92 Shinigami92 changed the base branch from main to feat-random-numeric April 8, 2022 10:11
@Shinigami92 Shinigami92 force-pushed the 348-improve-datatype-bigint branch from a77f218 to ae7752a Compare April 8, 2022 10:12
@import-brain import-brain added the needs rebase There is a merge conflict label Apr 10, 2022
@Shinigami92 Shinigami92 force-pushed the feat-random-numeric branch from bebd2a8 to 5f7e874 Compare April 19, 2022 14:50
@Shinigami92 Shinigami92 force-pushed the 348-improve-datatype-bigint branch from 2593c87 to 9d6eec5 Compare April 19, 2022 14:54
@Shinigami92 Shinigami92 removed the needs rebase There is a merge conflict label Apr 19, 2022
@Shinigami92 Shinigami92 force-pushed the feat-random-numeric branch from 9edd2fb to 633f857 Compare April 20, 2022 12:37
@Shinigami92 Shinigami92 marked this pull request as ready for review April 24, 2022 17:14
@Shinigami92 Shinigami92 requested a review from a team as a code owner April 24, 2022 17:14
@Shinigami92 Shinigami92 requested review from a team, ST-DDT and pkuczynski April 24, 2022 17:15
ST-DDT
ST-DDT previously approved these changes Apr 24, 2022
View reviewed changes
@ST-DDT ST-DDT requested a review from a team April 24, 2022 22:09
@Shinigami92 @ST-DDT
feat: reimplement datatype.bigInt
46bf568 
Co-authored-by: ST-DDT <ST-DDT@gmx.de>
@Shinigami92 Shinigami92 force-pushed the 348-improve-datatype-bigint branch from d7048cc to 46bf568 Compare May 18, 2022 17:23
@Shinigami92 Shinigami92 requested a review from ST-DDT May 18, 2022 17:24
@ST-DDT ST-DDT requested a review from a team May 18, 2022 22:17
@Shinigami92 Shinigami92 enabled auto-merge (squash) May 23, 2022 06:58
@Shinigami92 Shinigami92 merged commit 1793385 into main May 23, 2022
@ST-DDT ST-DDT deleted the 348-improve-datatype-bigint branch May 23, 2022 06:59
xDivisionByZerox
xDivisionByZerox reviewed May 23, 2022
View reviewed changes
src/modules/datatype/index.ts
max = BigInt(options.max ?? min + BigInt(999999999999999));
} else {
min = BigInt(0);
max = BigInt(options ?? 999999999999999);
Copy link
Member

@xDivisionByZerox xDivisionByZerox May 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Why can you pass options directly here? MDN documentation states that the value can "May be a string or an integer". So why the boolean or the object?

I've never worked with BigInt so sorry if this makes no sense.

xDivisionByZerox
xDivisionByZerox reviewed May 23, 2022
View reviewed changes
src/modules/datatype/index.ts
bigInt(
options?:
| bigint
| boolean
Copy link
Member

Choose a reason for hiding this comment

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

What would faker.datatype.bigInt(false) return? I don't understand what advantage we have from accepting a boolean parameter.

Copy link
Member Author

Choose a reason for hiding this comment

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

image

It's just the official constructor argument

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xDivisionByZerox xDivisionByZerox xDivisionByZerox left review comments

@prisis prisis prisis approved these changes

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

@pkuczynski pkuczynski Awaiting requested review from pkuczynski

Assignees

@ST-DDT ST-DDT

@Shinigami92 Shinigami92

Labels
c: feature Request for new feature p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
None yet
Milestone
v7 - Current Major
Development

Successfully merging this pull request may close these issues.

datatype.bigint takes value and gives it just back
6 participants
@Shinigami92 @ST-DDT @pkuczynski @prisis @xDivisionByZerox @import-brain