Skip to content

Comments

feat: add random.string function to eldritch#768

Merged
flemingcaleb merged 3 commits intomainfrom
add-eldritch-random-string
May 4, 2024
Merged

feat: add random.string function to eldritch#768
flemingcaleb merged 3 commits intomainfrom
add-eldritch-random-string

Conversation

@flemingcaleb
Copy link
Collaborator

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

@flemingcaleb flemingcaleb requested a review from hulto May 3, 2024 02:43
@flemingcaleb flemingcaleb self-assigned this May 3, 2024
Copy link
Collaborator

@hulto hulto left a comment

Choose a reason for hiding this comment

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

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 👏


### random.string

`random.string(length: uint) -> str`
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

pub fn info(starlark_heap: &Heap, pid: Option<usize>) -> Result<Dict> {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or maybe a String instead of Vec might be more ergonomic

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also please add the default character set it will use.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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()
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

@hulto
Copy link
Collaborator

hulto commented May 3, 2024

Oh one last ask please validate that no compiler warnings are generated when building for debug or release

@flemingcaleb
Copy link
Collaborator Author

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

@flemingcaleb flemingcaleb force-pushed the add-eldritch-random-string branch from 03dc8d7 to e33c4a1 Compare May 4, 2024 20:43
@flemingcaleb flemingcaleb merged commit 1dd95fd into main May 4, 2024
@flemingcaleb flemingcaleb deleted the add-eldritch-random-string branch May 4, 2024 21:06
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.

2 participants