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.

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

feat(number): add binary and octal random number generation #1708

Conversation

pladreyt
Copy link
Contributor

@pladreyt pladreyt commented Jan 2, 2023

This PR implements binary and octal random number generation.

Issue: #184

PR that implements this feature in string module: #1710

@pladreyt pladreyt requested a review from a team as a code owner January 2, 2023 10:27
Copy link
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

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

Could you please move the methods above the hex method so that the methods are a bit more in order.
Binary(2), Octal(8), Hex(16)

Should we also create these methods in the stri g module to generate them in a specific length?

@codecov
Copy link

codecov bot commented Jan 2, 2023

Codecov Report

Merging #1708 (eac739d) into next (84b3c20) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #1708   +/-   ##
=======================================
  Coverage   99.63%   99.63%           
=======================================
  Files        2249     2249           
  Lines      240424   240482   +58     
  Branches     1079     1083    +4     
=======================================
+ Hits       239549   239607   +58     
  Misses        854      854           
  Partials       21       21           
Impacted Files Coverage Δ
src/modules/number/index.ts 100.00% <100.00%> (ø)

test/number.spec.ts Outdated Show resolved Hide resolved
test/number.spec.ts Outdated Show resolved Hide resolved
test/number.spec.ts Outdated Show resolved Hide resolved
@ST-DDT ST-DDT added c: feature Request for new feature p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug m: number Something is referring to the number module labels Jan 2, 2023
@pladreyt
Copy link
Contributor Author

pladreyt commented Jan 2, 2023

Could you please move the methods above the hex method so that the methods are a bit more in order. Binary(2), Octal(8), Hex(16)

Should we also create these methods in the stri g module to generate them in a specific length?

Sure, will reorder the methods. Should I create another PR to add them in the string module as well ?

@ST-DDT
Copy link
Member

ST-DDT commented Jan 2, 2023

Should I create another PR to add them in the string module as well ?

Yes, I think that would be the easiest solution.
The PR that gets merged later will have to add @see links to both though. I leave the decision to you whether you want to create the other PR now or later.

test/number.spec.ts Outdated Show resolved Hide resolved
test/number.spec.ts Outdated Show resolved Hide resolved
test/number.spec.ts Outdated Show resolved Hide resolved
Shinigami92
Shinigami92 previously approved these changes Jan 2, 2023
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.

Please solve @ST-DDT suggestions, but already a good-to-go from my side

@Shinigami92 Shinigami92 enabled auto-merge (squash) January 3, 2023 07:56
@Shinigami92 Shinigami92 merged commit d3229fc into faker-js:next Jan 3, 2023
pladreyt added a commit to pladreyt/faker that referenced this pull request Jan 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: feature Request for new feature m: number Something is referring to the number module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants