-
Notifications
You must be signed in to change notification settings - Fork 226
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
Fix the trimming of trailing characters. #351
Conversation
When the string ended with extended grapheme clusters, the method crashed because of the force-unwrapping. This refactoring adds support for handling extended grapheme clusters.
2277680
to
ef49d43
Compare
var string = self.bridge() | ||
let endIndex = string.unicodeScalars.endIndex | ||
for offset in 1...string.unicodeScalars.count { | ||
let index = string.unicodeScalars.index(endIndex, offsetBy: -offset) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This creates the index from the end of the string every time.
Can you reuse the index created in the previous loop to reduce computational cost?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're right. Thanks for the input. I think there's even a much easier implementation (at least for me it's much easier to wrap my head around 😉 ):
var unicodeScalars = self.bridge().unicodeScalars
while let scalar = unicodeScalars.last {
if !characterSet.contains(scalar) {
return String(unicodeScalars)
}
unicodeScalars.removeLast()
}
Do you have concerns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Could you please add a changelog entry? Thanks! |
Sure! Done. |
Thanks @Lukas-Stuehrk! |
You're welcome. I'm happy to help on such a handy and important piece in the Swift tooling. Thanks for the fast reviews, good feedback and all your work. |
When the string ended with extended grapheme clusters, the method
crashed because of the force-unwrapping. This refactoring adds support
for handling extended grapheme clusters.
This fixes #350