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

Multiple Random Numbers Support #5518

Merged
merged 15 commits into from
Sep 22, 2024
Merged

Conversation

Ankoki
Copy link
Contributor

@Ankoki Ankoki commented Mar 14, 2023

Took 26 minutes

Description

Support multiple random numbers being retrieved.
I put them in one pattern as the possibility of a person putting 1 random number is there, so an optional [s] after numbers/integers is still wanted.

Target Minecraft Versions: Any
Requirements: N/A
Related Issues: #5517

@UnderscoreTud UnderscoreTud added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Mar 14, 2023
Ankoki added 2 commits March 14, 2023 18:31
Took 15 minutes
Took 3 minutes
return new Double[] {ll + rand.nextDouble() * (uu - ll)};
Double[] doubles = new Double[amount];
for (int i = 0; i < amount; i++)
doubles[i] = lower + rand.nextDouble() * (upper - lower);
Copy link
Member

Choose a reason for hiding this comment

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

This is not guaranteed to be within the bounds due to number rounding, you should check whether the new value is less than upper.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i see, this is also another thing that was unchanged by me, presuming i just need to make sure the bigger one is under upper?

@TheLimeGlass
Copy link
Collaborator

@Ankoki conflicts

Copy link
Member

@Moderocky Moderocky left a comment

Choose a reason for hiding this comment

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

My changes weren't addressed since last time, don't know why I was re-requested.

@TheLimeGlass
Copy link
Collaborator

My changes weren't addressed since last time, don't know why I was re-requested.

It was a misclick, but there are follow up questions/statements.

@Moderocky
Copy link
Member

Moderocky commented Jun 28, 2023

It was a misclick, but there are follow up questions/statements.

Alright, having looked I think these are probably something that needs more team input: do we keep the original (but potentially wrong) behaviour or do we improve it but risk a tiny breaking change?

@Ankoki
Copy link
Contributor Author

Ankoki commented Jun 28, 2023

Apologies I did completely forget about this PR, when i'm back from my trip i'll add the requested change.
In terms of the change, I believe if an empty variable is provided, it should be classed as 0, it makes the most sense to me. I don't believe it would break the current working as any person who doesn't get a list back when using nothing is not benefiting from the expression at all.

@TheLimeGlass
Copy link
Collaborator

@Ankoki conflicts

Copy link
Member

@AyhamAl-Ali AyhamAl-Ali left a comment

Choose a reason for hiding this comment

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

Nice PR ⚡

@Name("Random Number")
@Description({"A random number or integer between two given numbers. Use 'number' if you want any number with decimal parts, or use use 'integer' if you only want whole numbers.",
@Name("Random Numbers")
@Description({"A given amount of random numbers or integers between two given numbers. Use 'number' if you want any number with decimal parts, or use use 'integer' if you only want whole numbers.",
"Please note that the order of the numbers doesn't matter, i.e. <code>random number between 2 and 1</code> will work as well as <code>random number between 1 and 2</code>."})
@Examples({"set the player's health to a random number between 5 and 10",
Copy link
Member

Choose a reason for hiding this comment

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

Can you add an example for decimals as well, also use same formatting as description

@Moderocky Moderocky force-pushed the master branch 2 times, most recently from bd134d0 to 3f08853 Compare September 16, 2023 16:59
@Moderocky Moderocky changed the base branch from master to dev/feature September 18, 2023 10:21
@sovdeeth
Copy link
Member

@Ankoki Do you intend to continue working on this, or should we make the requested changes for you?

sovdeeth and others added 2 commits March 19, 2024 18:16
Co-authored-by: Ayham Al Ali <20037329+AyhamAl-Ali@users.noreply.github.com>
@sovdeeth
Copy link
Member

If it's not already clear, I'm taking this over :)

@sovdeeth sovdeeth added the feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. label Sep 15, 2024
@sovdeeth sovdeeth merged commit 14b9ed3 into SkriptLang:dev/feature Sep 22, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature request, an issue about something that could be improved, or a PR improving something. feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants