-
-
Notifications
You must be signed in to change notification settings - Fork 171
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 charset based variants for trimming, rename from left/right to start/end #696
base: main
Are you sure you want to change the base?
Conversation
pub fn trim_start_rtl_test() { | ||
" עברית " | ||
|> string.trim_start | ||
|> should.equal("עברית ") | ||
} | ||
|
||
pub fn trim_end_rtl_test() { | ||
" עברית " | ||
|> string.trim_end | ||
|> should.equal(" עברית") | ||
} |
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.
These tests surprised me, as a native english speaker (I do not speak any RTL languages), but it seems to match Rust's implementation, so I'd be surprised if it was wrong? Happy to be corrected!
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.
Presumably it's because the RTL text is only in the middle, surrounded by LTR text.
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.
Ah yeah - I've gone looking and not been able to find an example of a test case that has RTL whitespace I can use to test here. Maybe there is and I'm just not finding it
45d1a2c
to
2602480
Compare
2602480
to
db3d5c5
Compare
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.
Only trim_start
and trim_end
was approved for implementation, please remove the others. Thank you.
@lpil I can remove the aliases, sure. Though might you consider keeping |
Any other changes and additions can be discussed in the issue. There's some unanswered questions there today. |
Fixes #587
The primary intent of this PR is to add
trim_with
,trim_start_with
andtrim_end_with
to thestring
module, which will trim the specified chars from both ends of a string, the start of a string, or the end of the string, respectively.Given this change from
left/right
tostart/end
(to be more clear about RTL behavior), I renamed the existingtrim_left
/trim_end
totrim_start
andtrim_end
, and deprecated the old ones. Do let me know if this was an over-reach on my part!The names here are certainly changeable here, I just picked ones that made sense to me :)