Skip to content

Add many GString/StringName methods #980

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

Merged
merged 9 commits into from
Dec 27, 2024
Merged

Conversation

Bromeon
Copy link
Member

@Bromeon Bromeon commented Dec 17, 2024

Populates GString + StringName with the majority of generated methods from Godot.

I made multiple API design choices to make string operations more idiomatic in Rust:

  • Option::None instead of -1 for "not found".
  • Builder pattern for split + find, instead of proliferation of find/findn/rfind/rfindn and x2 with _from index.
  • Use usize instead of i64 where integers are unsigned offsets/indices.
  • Use std::ops::RangeBounds instead of inconsistent from, (from, to), (from, len) conventions used in Godot.
  • Use char instead of i64 when a Unicode code point is required.
  • Use std::cmp::Ordering when the result is -1, 0 or 1 depending on ordering relation.
  • Add a with_* overload for rarely used alternatives (format + format_with_placeholder).
  • Rename in case of ambiguity (match -> match_glob).

Added unit tests for most things.

For the [natural|file][no]casecmp_to methods, I currently left the original methods, but technically they could use a builder as well. Might change in the future...

@Bromeon Bromeon added feature Adds functionality to the library c: core Core components c: engine Godot classes (nodes, resources, ...) labels Dec 17, 2024
@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-980

@Bromeon Bromeon force-pushed the feature/string-generated-methods branch 2 times, most recently from c0c2580 to 5d7a7fe Compare December 17, 2024 22:25
@Bromeon Bromeon force-pushed the feature/string-generated-methods branch from 5d7a7fe to 6c89d57 Compare December 17, 2024 22:26
Copy link
Member

@lilizoey lilizoey left a comment

Choose a reason for hiding this comment

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

I think it would be nice if we could like have some feature where we can change at least the return value of some auto-generated functions.

Like maybe is_builtin_method_exposed could return an Option<ExposeOptions> or something, and ExposeOptions is an enum with variants like

enum ExposeOptions {
    Default,
    Ordering, // -1 = Less, 0 = Equal, 1 = Greater
    Option, 
    Usize, 
}

Which will have the code attempt to change the return value to that type? idk

maybe at least group the functions somehow for now by which should remain unchanged and which should have their signature modified so we can more easily change it after implementing something like the above.

Otherwise i think this is pretty good.

Comment on lines 82 to 83
/// _Godot equivalent: `length`_
pub fn len(&self) -> usize {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// _Godot equivalent: `length`_
pub fn len(&self) -> usize {
/// _Godot equivalent: `length`_
#[doc(alias = "length")]
pub fn len(&self) -> usize {

Comment on lines 238 to 243
| ("String", "casecmp_to")
| ("String", "nocasecmp_to")
| ("String", "naturalcasecmp_to")
| ("String", "naturalnocasecmp_to")
| ("String", "filecasecmp_to")
| ("String", "filenocasecmp_to")
Copy link
Member

Choose a reason for hiding this comment

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

Should these functions return a Ordering? They do return -1, 0, 1 corresponding to less/equal/greater. Same with StringName below

Comment on lines 244 to 246
| ("String", "get_slice")
| ("String", "get_slicec")
| ("String", "get_slice_count")
Copy link
Member

Choose a reason for hiding this comment

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

There are a some functions like these that can fail that should maybe return Option<T> instead, though get_slice actually is a bit weird in that there are two failure cases that have different behavior.

@Bromeon Bromeon force-pushed the feature/string-generated-methods branch from 38385b7 to 07a553c Compare December 24, 2024 22:42
@Bromeon Bromeon force-pushed the feature/string-generated-methods branch 3 times, most recently from 1c1e47f to f70341d Compare December 27, 2024 14:53
@Bromeon Bromeon force-pushed the feature/string-generated-methods branch from f70341d to e63f68b Compare December 27, 2024 14:57
@Bromeon Bromeon force-pushed the feature/string-generated-methods branch 3 times, most recently from 16afc0a to 06b0e49 Compare December 27, 2024 17:28
@Bromeon Bromeon force-pushed the feature/string-generated-methods branch from 06b0e49 to 2055bdf Compare December 27, 2024 17:33
@Bromeon
Copy link
Member Author

Bromeon commented Dec 27, 2024

So, I added a ton of methods, including several manually redefined ones. See initial post for API design.

@Bromeon Bromeon added this pull request to the merge queue Dec 27, 2024
Merged via the queue into master with commit 8eaf184 Dec 27, 2024
15 checks passed
@Bromeon Bromeon deleted the feature/string-generated-methods branch December 27, 2024 17:55
@Bromeon Bromeon mentioned this pull request Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: core Core components c: engine Godot classes (nodes, resources, ...) feature Adds functionality to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants