string.strip_prefix and string.strip_suffix implementation proposal #771
Conversation
|
I'd really like having these functions in the standard lib, thank you!! Unfortunately, your implementation is not correct: strip_prefix("👩👩👧👦", prefix: "👩")
// --> Ok("")
It is also slow - you loop over the strings multiple times, performing grapheme segmentation twice. Once you know that that a string has a prefix, you can immediately slice the source string based on its code unit length. We don't usually bump the version in PRs; this is left to the maintainers. You should add your changes to the changelog though! ~ yoshi 💜 |
|
I changed the implementation to a JS side and Erlang side implementations instead of Gleam. I didn't know where to put my changes in the changelog other than in a new version, I let you decide what to do with the version number. Thanks for your feedback ! |
There was a problem hiding this comment.
Thank you ~ 💜
Looks good to me now; I left some comments inline! Also please keep in mind that the wonderful @lpil is the maintainer not me, I'm just a random person on the internet with ~opinions~ ^^
There was a problem hiding this comment.
Hello! Thank you for this, it will be super useful.
How come the implementation is in Erlang and JavaScript? Given their complexity and the algorithm being the same across both it should be in Gleam unless there's some particular reason to do otherwise.
The new implementation of starts_with and ends_with is slower due to doing wasted work, please revert that change.
You'll need to rebase to remove the merge commit, they are not permitted in Gleam repos.
Thanks again!!
| string.strip_suffix("👨👩👧👦", suffix: "👦") | ||
| |> should.equal( | ||
| Ok( | ||
| string.from_utf_codepoints([ |
There was a problem hiding this comment.
How come this isn't a string literal?
| } | ||
|
|
||
| return new Error(undefined) | ||
| } |
There was a problem hiding this comment.
Files have to have a newline at the end to be valid!
| @@ -569,4 +577,4 @@ slice(String, Index, Length) -> | |||
| case string:slice(String, Index, Length) of | |||
| X when is_binary(X) -> X; | |||
| X when is_list(X) -> unicode:characters_to_binary(X) | |||
| end. | |||
| end. | |||
There was a problem hiding this comment.
Files have to have a newline at the end to be valid!
| string_strip_prefix(String, <<>>) when is_binary(String) -> {ok, String}; | ||
| string_strip_prefix(String, _) when is_binary(String), String == <<>> -> {error, nil}; | ||
| string_strip_prefix(String, Prefix) when is_binary(String), is_binary(Prefix), byte_size(Prefix) > byte_size(String) -> {error, nil}; | ||
| string_strip_prefix(String, Prefix) when is_binary(String), is_binary(Prefix) -> |
There was a problem hiding this comment.
Remove all the unneeded guards please 🙏
Hi !
Here's a simple proposal about the implementation of
string.strip_prefixandstring.strip_suffixThis PR references issue #743 .
Feel free to comment.