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

Document pick_random for empty arrays #80694

Merged
merged 1 commit into from
Aug 21, 2023

Conversation

novalis
Copy link
Contributor

@novalis novalis commented Aug 16, 2023

No description provided.

@Sauermann
Copy link
Contributor

Executing pick_random on an empty array results in the following error message:

E 0:00:02:0415   Control.gd:64 @ _on_button_2_pressed(): Can't take value from empty array.
  <C++ Error>    Condition "_p->array.size() == 0" is true. Returning: Variant()
  <C++ Source>   core/variant/array.cpp:329 @ pick_random()
  <Stack Trace>  Control.gd:64 @ _on_button_2_pressed()

So I'm not entirely convinced, that documenting a behavior which causes an error message is warranted.

@AThousandShips
Copy link
Member

AThousandShips commented Aug 16, 2023

We can possibly document that it requires a non empty array, though that should be obvious, as it says "from"

@Sauermann
Copy link
Contributor

Sounds good. The documentation of Array.back and Array.front could be used for that.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

LGTM, clear and concise

@akien-mga
Copy link
Member

Could you squash the commits? See PR workflow for instructions.

@akien-mga akien-mga modified the milestones: 4.x, 4.2 Aug 18, 2023
@akien-mga akien-mga changed the title Document pick_random for empty arrays Document pick_random for empty arrays Aug 21, 2023
@akien-mga akien-mga merged commit 56dd0ed into godotengine:master Aug 21, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants