-
Notifications
You must be signed in to change notification settings - Fork 259
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 step rand command #1054
Add step rand command #1054
Conversation
This commit adds a new command `step rand` that generates random strings
Overall I like this this addition, but I do want to say that I think it's strange to use dictionary.length and dice.length as the same default value. It's nice for now, but it's going to be a headache to decouple later. |
I like the addition too, but would suggest to put it under |
command/rand/rand.go
Outdated
|
||
The following special formats are also supported: | ||
|
||
* dice: generates a random number between 1 and 6 or the given argument, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, this is a multiple, but it's just a single die being rolled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kinda agree here - I assumed the command was going to roll multiple dice for me before reading the example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why even have 'die' or 'dice' as a special format? Like, why not just use step rand int
or step rand num
and then allow to specify the start and end inclusive or exclusive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like the only reason to use dice is because the number 6 matches the number of default words in the dictionary format - and I don't think that relation makes sense. I already commented about that earlier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
step rand int
is a good addition that can be added later, but it requires more flags. 🎲 is fun. A die of 6 is the most common one. The match between the number of words in the dictionary and the default die is just a coincidence. Six looked like a good default.
Co-authored-by: Max <[email protected]>
Co-authored-by: Herman Slatman <[email protected]>
command/rand/rand.go
Outdated
|
||
The following special formats are also supported: | ||
|
||
* dice: generates a random number between 1 and 6 or the given argument, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* dice: generates a random number between 1 and 6 or the given argument, | |
* dice: generates a random number between 1 and 6 (by default), or the 1 and the provided argument. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed with 12263b8
This commit changes the examples from `step rand ..` to `step crypto rand ...`
Love this 🎲 and I will use it! |
This commit adds a new command
step rand
that generates random strings