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

Add function for generating random values #2049

Merged
merged 10 commits into from
May 18, 2024
Merged

Add function for generating random values #2049

merged 10 commits into from
May 18, 2024

Conversation

laniakea64
Copy link
Contributor

Adds pick() function as discussed in #1991 (comment)

I'm still relatively new to Rust, so code review would be much appreciated 🙂

cc @t3hmrman

README.md Outdated
Comment on lines 1497 to 1498
- `pick(len)`<sup>master</sup> - Generate a string of `len` random lowercase hexadecimal digit characters.
- `pick(len, alphabet)`<sup>master</sup> - Generate a random string of length `len` consisting of characters found in `alphabet`.
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're into it, you could add some examples of the use to the examples folder -- maybe under the existing kitchen sink ones?

Copy link
Owner

Choose a reason for hiding this comment

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

I think it's fine not to. I don't really require adding examples for every new function. An example can be added to the readme though.

src/function.rs Outdated Show resolved Hide resolved
src/function.rs Outdated Show resolved Hide resolved
src/function.rs Outdated Show resolved Hide resolved
src/function.rs Outdated Show resolved Hide resolved
src/function.rs Outdated Show resolved Hide resolved
@t3hmrman
Copy link
Contributor

Hey @laniakea64 thanks for the quick turnaround on this! One more thing that would be awesome are some tests! There's not much you can test (especially as this is real random and not pseudo random), but at the very least the string formation/length might be worth testing!

I wonder if you can easily test entropy (I assume GH runners have enough and this won't be a problem)

@laniakea64
Copy link
Contributor Author

Hey @laniakea64 thanks for the quick turnaround on this!

You're welcome, thanks for your review!

One more thing that would be awesome are some tests! There's not much you can test (especially as this is real random and not pseudo random), but at the very least the string formation/length might be worth testing!

Done.
(I have no actual idea what I'm doing in this area, I just tried to follow the pattern of how existing functions tests were written and this change passed cargo clippy --all-targets --all-features and cargo test locally.)

I wonder if you can easily test entropy (I assume GH runners have enough and this won't be a problem)

I have no idea how to even start there, sorry 😨

Copy link
Owner

@casey casey left a comment

Choose a reason for hiding this comment

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

Looks good! I think we should this function choose. I was poking around, and that seems to be the most common name.

README.md Outdated
Comment on lines 1497 to 1498
- `pick(len)`<sup>master</sup> - Generate a string of `len` random lowercase hexadecimal digit characters.
- `pick(len, alphabet)`<sup>master</sup> - Generate a random string of length `len` consisting of characters found in `alphabet`.
Copy link
Owner

Choose a reason for hiding this comment

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

I think it's fine not to. I don't really require adding examples for every new function. An example can be added to the readme though.

src/function.rs Outdated Show resolved Hide resolved
src/function.rs Outdated Show resolved Hide resolved
src/function.rs Outdated Show resolved Hide resolved
@casey casey enabled auto-merge (squash) May 18, 2024 23:28
@casey casey merged commit 7fa6ed8 into casey:master May 18, 2024
5 checks passed
@casey
Copy link
Owner

casey commented May 18, 2024

LGTM!

I just merged #2054, which adds some predefined constants, including HEX which are the lowercase hexidecimal digits. Because HEX is now available, I made choose require alphabet, since it's very short, and maybe a little less confusing to write choose('10', HEX) instead of choose('10') where hex is implied. I made a couple of other small changes to the function, including using map_err to produce the error, and (0..n).map(…).collect() to produce the string, to avoid the for loop.

I actually think that, in the general case, pre-allocating the string length isn't useful. The length of a string is in bytes, but characters may be multiple bytes when encoded in UTF-8.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants