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 step rand command #1054

Merged
merged 6 commits into from
Nov 3, 2023
Merged

Add step rand command #1054

merged 6 commits into from
Nov 3, 2023

Conversation

maraino
Copy link
Collaborator

@maraino maraino commented Oct 26, 2023

This commit adds a new command step rand that generates random strings

This commit adds a new command `step rand` that generates random strings
@maraino maraino requested a review from dopey October 26, 2023 22:14
@github-actions github-actions bot added the needs triage Waiting for discussion / prioritization by team label Oct 26, 2023
command/rand/rand.go Outdated Show resolved Hide resolved
command/rand/rand.go Outdated Show resolved Hide resolved
@dopey
Copy link
Contributor

dopey commented Oct 27, 2023

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.

@hslatman
Copy link
Member

hslatman commented Oct 27, 2023

I like the addition too, but would suggest to put it under step crypto -> step crypto rand.

command/rand/rand.go Outdated Show resolved Hide resolved

The following special formats are also supported:

* dice: generates a random number between 1 and 6 or the given argument,
Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@dopey @hslatman should I change the format name to die?

Copy link
Contributor

@dopey dopey Oct 27, 2023

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.

Copy link
Contributor

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.

Copy link
Collaborator Author

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.

maraino and others added 2 commits October 27, 2023 10:04

The following special formats are also supported:

* dice: generates a random number between 1 and 6 or the given argument,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed with 12263b8

@maraino maraino requested review from hslatman and dopey October 27, 2023 18:24
This commit changes the examples from `step rand ..` to `step crypto
rand ...`
dopey
dopey previously approved these changes Oct 27, 2023
@tashian
Copy link
Contributor

tashian commented Oct 27, 2023

Love this 🎲 and I will use it!
Thank you for including emoji 🥰

@maraino maraino merged commit ba4214f into master Nov 3, 2023
14 checks passed
@maraino maraino deleted the mariano/rand branch November 3, 2023 17:25
@hslatman hslatman added this to the v0.25.1 milestone Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs triage Waiting for discussion / prioritization by team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants