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(string): add binary and octal random string generation #1710

Conversation

pladreyt
Copy link
Contributor

@pladreyt pladreyt commented Jan 2, 2023

This PR implements binary and octal random string generation.

Issue: #184

PR that implements this feature in number module: #1708

@pladreyt pladreyt requested a review from a team as a code owner January 2, 2023 18:23
@import-brain import-brain added c: feature Request for new feature p: 1-normal Nothing urgent m: string Something is referring to the string module labels Jan 2, 2023
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.

Especially octal and binary seem to have a multitude of common prefixes, should we use '' as the default prefix for all of them?

Only one of our methods uses a non-empty prefix (etheriumAddress), all other 5 methods use ''.

src/modules/string/index.ts Outdated Show resolved Hide resolved
src/modules/string/index.ts Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jan 2, 2023

Codecov Report

Merging #1710 (6ece169) into next (d3229fc) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #1710   +/-   ##
=======================================
  Coverage   99.63%   99.63%           
=======================================
  Files        2249     2249           
  Lines      240482   240573   +91     
  Branches     1083     1095   +12     
=======================================
+ Hits       239607   239698   +91     
  Misses        854      854           
  Partials       21       21           
Impacted Files Coverage Δ
src/modules/number/index.ts 100.00% <100.00%> (ø)
src/modules/string/index.ts 100.00% <100.00%> (ø)

@ST-DDT ST-DDT added the s: needs decision Needs team/maintainer decision label Jan 2, 2023
@pladreyt
Copy link
Contributor Author

pladreyt commented Jan 2, 2023

Especially octal and binary seem to have a multitude of common prefixes, should we use '' as the default prefix for all of them?

Only one of our methods uses a non-empty prefix (etheriumAddress), all other 5 methods use ''.

Also the hexadecimal method uses a non-empty prefix, I was using it as a base. Was keeping the prefix for consistency with that method, but I can set it to ''

@ST-DDT
Copy link
Member

ST-DDT commented Jan 3, 2023

Please add @see to and from the number module.

@pladreyt
Copy link
Contributor Author

pladreyt commented Jan 3, 2023

Please add @see to and from the number module.

Sure!

@pladreyt pladreyt force-pushed the feature-184-add-binary-and-octal-random-string-generation branch 3 times, most recently from c16ef52 to f0836be Compare January 3, 2023 17:59
@pladreyt pladreyt force-pushed the feature-184-add-binary-and-octal-random-string-generation branch from f0836be to 28d4c8a Compare January 3, 2023 18:00
@pladreyt pladreyt requested review from ST-DDT and Shinigami92 and removed request for ST-DDT and Shinigami92 January 3, 2023 18:21
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.

Two minor changes, then we are good to go from my side.

src/modules/string/index.ts Outdated Show resolved Hide resolved
src/modules/string/index.ts Outdated Show resolved Hide resolved
@ST-DDT ST-DDT removed the s: needs decision Needs team/maintainer decision label Jan 3, 2023
@ST-DDT ST-DDT added the s: accepted Accepted feature / Confirmed bug label Jan 3, 2023
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.

Thanks for your contribution.

@ST-DDT ST-DDT requested review from Shinigami92 and a team January 4, 2023 17:37
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: string Something is referring to the string 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