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 charset based variants for trimming, rename from left/right to start/end #696

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

ollien
Copy link

@ollien ollien commented Oct 6, 2024

Fixes #587

The primary intent of this PR is to add trim_with, trim_start_with and trim_end_with to the string 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 to start/end (to be more clear about RTL behavior), I renamed the existing trim_left/trim_end to trim_start and trim_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 :)

Comment on lines +173 to +183
pub fn trim_start_rtl_test() {
" עברית "
|> string.trim_start
|> should.equal("עברית ")
}

pub fn trim_end_rtl_test() {
" עברית "
|> string.trim_end
|> should.equal(" עברית")
}
Copy link
Author

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!

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=3c96c56a9a7da1a7c70898000531ba38

Copy link
Contributor

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.

Copy link
Author

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

src/gleam_stdlib.mjs Outdated Show resolved Hide resolved
Copy link
Member

@lpil lpil left a 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.

@ollien
Copy link
Author

ollien commented Oct 7, 2024

@lpil I can remove the aliases, sure. Though might you consider keeping trim_with? Having the start/end variants and the bidirectional variant seems only natural.

@lpil
Copy link
Member

lpil commented Oct 7, 2024

Any other changes and additions can be discussed in the issue. There's some unanswered questions there today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

More string trimming capabilities
3 participants