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: Add random binary and octal number generation #191

Closed
wants to merge 9 commits into from

Conversation

mbokinala
Copy link
Contributor

closes #184

@netlify
Copy link

netlify bot commented Jan 17, 2022

✔️ Deploy Preview for vigilant-wescoff-04e480 ready!

🔨 Explore the source changes: c8b0625

🔍 Inspect the deploy log: https://app.netlify.com/sites/vigilant-wescoff-04e480/deploys/61e77b26c7a2e90008e8ed85

😎 Browse the preview: https://deploy-preview-191--vigilant-wescoff-04e480.netlify.app

@Shinigami92 Shinigami92 added the p: 1-normal Nothing urgent label Jan 17, 2022
@Shinigami92 Shinigami92 added this to the v6.1.0 milestone Jan 17, 2022
src/datatype.ts Outdated
* @param count defaults to 1
*/
octal(count: number = 1): string {
let wholeString = '';
Copy link

Choose a reason for hiding this comment

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

Suggested change
let wholeString = '';
let octalString = '';

src/datatype.ts Outdated
* @param count defaults to 1
*/
binary(count: number = 1): string {
let wholeString = '';
Copy link

Choose a reason for hiding this comment

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

Suggested change
let wholeString = '';
let binaryString = '';


it('generates a random binary string', function () {
var hex = binary(12);
assert.ok(hex.match(/^(0b)[01]+$/i));
Copy link

Choose a reason for hiding this comment

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

Suggested change
assert.ok(hex.match(/^(0b)[01]+$/i));
assert.ok(hex.match(/^0b[01]{12}$/i));


it('generates a single binary charcter when no additional argument is provided', function () {
var binaryNum = binary();
assert.ok(binaryNum.match(/^(0b)[01]{1}$/i));
Copy link

Choose a reason for hiding this comment

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

Suggested change
assert.ok(binaryNum.match(/^(0b)[01]{1}$/i));
assert.ok(binaryNum.match(/^0b[01]$/i));

src/datatype.ts Outdated
* @method faker.datatype.octal
* @param count defaults to 1
*/
octal(count: number = 1): string {
Copy link

Choose a reason for hiding this comment

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

I'd suggest length, but it's OK for consistency with hexaDecimal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, I'll update this

docs/api/datatype.md Show resolved Hide resolved
poyea
poyea previously approved these changes Jan 18, 2022
Copy link

@poyea poyea left a comment

Choose a reason for hiding this comment

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

looks great!

src/datatype.ts Outdated
*/
hexaDecimal(count: number = 1): string {
hexaDecimal(length: number = 1): string {
let wholeString = '';
Copy link

Choose a reason for hiding this comment

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

Maybe hexaString too? It's very minor though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll edit it

@import-brain import-brain requested a review from a team January 23, 2022 05:03
@Shinigami92 Shinigami92 added the needs rebase There is a merge conflict label Jan 23, 2022
@import-brain import-brain added the c: feature Request for new feature label Mar 30, 2022
@Shinigami92
Copy link
Member

We're starting work on the v6.2 milestone, but this PR needs a rebase.
Please rebase and resolve all conflicts. Until then, we're moving it to the v6 milestone, which now serves as the "v6-Backlog".
If this PR no longer contains any conflicts and can certainly be a target for the next release, we will move it to the then active milestone v6.x.

@petrzjunior
Copy link

Could we also provide a version without the 0b, 0o, 0x prefix? Possibly via optional parameter.
In my usecase, I need to repeatedly write .substring(2) to remove it.

Buffer.from(faker.datatype.hexadecimal(20).substring(2), 'hex')

@Shinigami92
Copy link
Member

Shinigami92 commented May 13, 2022

Could we also provide a version without the 0b, 0o, 0x prefix? Possibly via optional parameter.

In my usecase, I need to repeatedly write .substring(2) to remove it.

Buffer.from(faker.datatype.hexadecimal(20).substring(2), 'hex')

Don't have to much time yet to go into details and I have other prios right now, but yes I would like to not prefix it by default at all but have just the option to add the prefix on demand via an option argument. Consistent for all of these functions!

=> #954

@xDivisionByZerox
Copy link
Member

I will close this PR since it's heavily outdated. I suggest opening another PR that at least has the 'new' test strategy and directory structure implemented.

If anyone really wants to keep this alive, feel free to reopen it.

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 needs rebase There is a merge conflict p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: add binary and octal random number generation
6 participants