Add replace_first for string, string_builder, and regex
#690
Conversation
|
Does it make any sense to make a more general |
I’d go for the longer but more explicit |
|
@giacomocavalieri I think replace_first could be more appropriate as well. @GearsDatapacks The replace functions default to just one, and for whatever reason they chose to implement replace all in this lib explicitly. I also don't see a use-case for supporting |
LagunaElectric
changed the title
replace_one for string, string_builder, and regexreplace_first for string, string_builder, and regex
Fair enough. Just thought I'd bring it up as an option |
|
@GearsDatapacks There were no tests for the regular replace versions, so I didn't add any. I can add them if needed! :) |
|
There is one for |
|
@GearsDatapacks LGTM |
| @@ -451,6 +455,13 @@ export function regex_replace(regex, original_string, replacement) { | |||
| return original_string.replaceAll(regex, replacement); | |||
| } | |||
|
|
|||
| export function regex_replace_first(regex, original_string, replacement) { | |||
| // Forcibly strip the g flag from the regex, if it's present | |||
There was a problem hiding this comment.
Nit: All JS regexes created in Gleam are global by default so this should always be true. See the compile_regex function above.
There was a problem hiding this comment.
This is why I have to forcibly strip the flag here, and why I confidently assume it will exist instead of checking for it.
|
Hello! Could you expand on the use cases you had for these functions? We are conservative with adding to the stdlib so tend to wait until we have a great demand for additions. |
|
@lpil I'm building a tool where the main focus is performing find/replace on large quantities of text (Think performing the same n transformations in sequence on 4000+ product description blocks). The ability to toggle whether this replaces one or all occurrences is standard. See replace functionality in for reference. Even in this library you had to add a param or flag to force it to default to global because the conventional default of both languages Gleam compiles to is replacing one. On that note, why was that the choice? Why not include this from the start? I would ask the same use-case question for only-global replace. Why not just expose the standard replace and let devs add the flag if needed? I whole-heartedly believe not having this already was a miss, and this PR fills the hole created by forcing global replace. I was surprised to be looking through the docs unable to find this functionality. I understand being conservative but I don't understand why this was omitted of all things. |
|
And you needed it for all three of these data structures in your specific case? |
|
No, I really only need the regex one but if it's there why wouldn't string have it too? As a developer I'd expect that.To implement it for string I had to implement it for string_builder as that's what the current string implementation uses under the hood. This way the use case is holistically solved and shouldn't require further attention moving forward. |
|
The standard library is very conservative, we only add functions when there's real needs that benefit a good amount of people. |
|
That's fair but I don't see how it's productive to this pull request. Should I remove the string implementations? Should I delete the whole request? What do you want from me here? This is a lot of back and forth for not a lot of code change. Also, what real needs benefitting a good amount of people are satisfied by only providing global replace? It's still not clear why there is only global replace. I wish you'd stop ignoring the question so I can understand better. I don't understand the pushback over such a simple addition. It's replace one. You didn't give it to us, so I added it. If you don't like it just say so, if it needs changing do a review, but at least give me more than a one line response "we're conservative". It'd be nice to know what that means in context of this PR. |
|
After further discussion it seems that the regex module may be separating from the standard lib. @lpil will likely do some thinking and decide what happens to this PR, whether it splits or dissolves. For now if anyone needs this functionality it's a small addition. I was able to add this to my project with very small Erl and JS files. |
Read the tin! The replace functions were all replace all style by default and there was no option at all to just replace the first occurrence. This rectifies that for both Erlang and JS targets. Let me know if it needs a name change or if this change isn't a good fit for the standard lib.