feat: add random.string function to eldritch#768
Conversation
hulto
left a comment
There was a problem hiding this comment.
Great first PR!
Stoked to add this function will open up a lot of functionality 🤌
Couple of thoughts - happy to chat if you have any questions.
Really great job 👏
docs/_docs/user-guide/eldritch.md
Outdated
|
|
||
| ### random.string | ||
|
|
||
| `random.string(length: uint) -> str` |
There was a problem hiding this comment.
Can you add an optional parameter to define the characters the function can draw from.
You can use the Option<Vec>
To achieve this.
An example of the option parameter in use can found in the process.info function.
There was a problem hiding this comment.
Or maybe a String instead of Vec might be more ergonomic
There was a problem hiding this comment.
Also please add the default character set it will use.
There was a problem hiding this comment.
Option added and documentation updated 😄
| use rand::{distributions::Alphanumeric, Rng}; // 0.8 | ||
|
|
||
| pub fn string(length: u64) -> Result<String> { | ||
| let s: String = rand::thread_rng() |
There was a problem hiding this comment.
This seems okay.
I noticed in the bool impl the rng is initialized separately.
let mut rng = rand_chacha::ChaCha20Rng::from_entropy();
Is there a reason to do it one way or another?
There was a problem hiding this comment.
I'm by no means an expert, but this documentation made me think that using the thread_rng is more secure since it re-seeded? I don't have a strong preference either way though and am happy to change if that is the preferred implementation
|
Oh one last ask please validate that no compiler warnings are generated when building for debug or release |
|
Confirmed that debug and release build without warnings with the stable-aarch64-apple-darwin toolchain. Trying to get the cross compile to x86 working but am having some troubles with getting the toolchain happy |
03dc8d7 to
e33c4a1
Compare
What type of PR is this?
/kind documentation
/kind feature
/kind eldritch-function
What this PR does / why we need it:
Adds random.string(uint) to eldritch to allow tome developers to generate random strings.
Which issue(s) this PR fixes:
N/A