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

Improve documentation for return value of Packed*Array.resize #104258

Merged
merged 1 commit into from
Mar 21, 2025

Conversation

Monochrome-debug
Copy link
Contributor

Changed the documentation to specify the return value of the resize() function because of this issue from the godot docs repo.

@Mickeon
Copy link
Contributor

Mickeon commented Mar 17, 2025

The Array equivalent of this method already has a more suitable way of describing this (since #69451):

Returns [constant OK] on success, or one of the other [enum Error] constants if this method fails.

I would suggest copying it. I don't think the way it is described in this PR (as of writing this) is clear or concise.

@Mickeon Mickeon added this to the 4.x milestone Mar 17, 2025
@AThousandShips AThousandShips changed the title Ammended documentation for return value of resize Improve documentation for return value of resize Mar 17, 2025
@AThousandShips AThousandShips changed the title Improve documentation for return value of resize Improve documentation for return value of Packed*Array.resize Mar 17, 2025
@Mickeon Mickeon requested a review from AThousandShips March 17, 2025 17:24
@Mickeon Mickeon requested a review from AThousandShips March 19, 2025 20:38
@Monochrome-debug
Copy link
Contributor Author

Should we merge now?

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

@AThousandShips AThousandShips modified the milestones: 4.x, 4.5 Mar 20, 2025
@akien-mga
Copy link
Member

For now this would be fine, but it could be worth looking into:

  • Properly describing why resize can fail and with what error code. From a quick look at CowData::resize, it can return ERR_INVALID_PARAMETER is size is negative, or ERR_OUT_OF_MEMORY if allocations fail.
  • Why don't these methods return Error instead of int? That would make it explicit that it's OK on success and not-OK otherwise.

@AThousandShips
Copy link
Member

AThousandShips commented Mar 20, 2025

The lack of Error is due to a limitation in the binding system for built-in types, unsure if tracked anywhere, but it's due to metadata not being passed (same reason as methods with int arguments are bound as int64_t in GDExtension for String for example)

Related:

Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>
Co-authored-by: Rémi Verschelde <rverschelde@gmail.com>
@akien-mga akien-mga force-pushed the documentation_change branch from 0a5ea67 to 44d10c5 Compare March 20, 2025 15:22
@akien-mga
Copy link
Member

Properly describing why resize can fail and with what error code. From a quick look at CowData::resize, it can return ERR_INVALID_PARAMETER is size is negative, or ERR_OUT_OF_MEMORY if allocations fail.

I went ahead and documented this, and made things further consistent with Array.

@akien-mga akien-mga merged commit 5595d82 into godotengine:master Mar 21, 2025
20 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

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.

None yet

4 participants