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 String.reverse method #78529

Merged
merged 1 commit into from Aug 16, 2023
Merged

Conversation

Chaosus
Copy link
Member

@Chaosus Chaosus commented Jun 21, 2023

Adds a simple method to return the copy of the string in reverse order. May be useful in some cases. Closes godotengine/godot-proposals#3503

@Chaosus Chaosus requested review from a team as code owners June 21, 2023 17:46
@Chaosus Chaosus added this to the 4.2 milestone Jun 21, 2023
@RedworkDE
Copy link
Member

  1. This implementation breaks some characters, eg "🇺🇦".reverse() == "🇦🇺"
  2. Generally you would want to keep unicode grapheme clusters intact and just reverse their order, but in some locales a grapheme cluster is basically an entire word, so there it's probably not what you want either.
  3. C# doesn't have this built-in because of exactly such problems, also its naive implementation would give different results.

TL/DR: this is a hard problem and I don't think this implementation does it justice.

@AThousandShips
Copy link
Member

I think the quirks of reversing should be documented as per the above mentioned

@dalexeev
Copy link
Member

@RedworkDE This is a good point, but note that some graphemes are composed of multiple code points:

print("🇺🇦".length()) # 2

I think we can just document that the reverse() method considers code points under individual characters, not graphemes (sorry if I misused the terms).

@AThousandShips
Copy link
Member

AThousandShips commented Jul 28, 2023

Something like:
Note: This treats individual Unicode codepoints as individual characters and does not respect clusters of codepoints, and may cause unexpected behaviour when reversing sequences including emoji, flags, etc.

@Zireael07
Copy link
Contributor

I don't know a reverse() implementation that does NOT break flags/emoji and the like. Should be documented and that's all.

@akien-mga
Copy link
Member

This implementation breaks some characters, eg "🇺🇦".reverse() == "🇦🇺"

I believe that's a feature, Australia is upside down :o) @BastiaanOlij

@bruvzg
Copy link
Member

bruvzg commented Aug 2, 2023

This implementation breaks some characters, eg "🇺🇦".reverse() == "🇦🇺"

None of the other String functions will handle composite emojis as a single character, so this is not an issue. Besides this, the result reversing string won't make any sense in many cases (any context dependent character combinations, combining diacritics etc.).

@YuriSizov
Copy link
Contributor

YuriSizov commented Aug 2, 2023

I agree that we shouldn't worry ourselves too much with how it handles complex unicode sequences and various composite languages. The resulting string won't be a valid word or sentence in practically any language. The application for this method lies somewhere else. Though the proposal doesn't elaborate why this is needed, only that it's needed.

If we seriously want to discuss if this is worth it or not, the proposal must provide some details for a use case of reversing a string, and doing so especially efficiently too, to be considered for a built-in (cc @me2beats). But we may also just YOLO this, as the naive implementation is pretty straightforward.

@me2beats
Copy link

me2beats commented Aug 9, 2023

I used string reverse mainly for searching replacing and sorting
with and without Regex

also can be used in some algorithms for example for procedural generation of names

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

The implementation is straightforward, and it seems like a useful addition for the String class.

It's always hard to know what's the threshold for accepting new helper methods in core, but this seems OK to me, and nobody voiced any specific concerns in the past few weeks.

@akien-mga akien-mga merged commit 20e24bd into godotengine:master Aug 16, 2023
13 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.

Add String.invert()/reverse()
9 participants