Add `replace_first` for string, string_builder, and regex

[LagunaElectric]

trafficstars

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.

[GearsDatapacks]

Does it make any sense to make a more general replace_n function, which allows you to specify any number of replacements to do? I can't immediately think of a usecase for it, but it could be useful at some point

[giacomocavalieri]

I thought about naming it replace_first but one was shorter

I’d go for the longer but more explicit replace_first, personally

[LagunaElectric]

@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 n matches, and the solution would be a little more involved than what I've currently done for little ROI time-wise.

[GearsDatapacks]

@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 n matches, and the solution would be a little more involved than what I've currently done for little ROI time-wise.

Fair enough. Just thought I'd bring it up as an option

[LagunaElectric]

@GearsDatapacks There were no tests for the regular replace versions, so I didn't add any. I can add them if needed! :)

[LagunaElectric]

There is one for string but not string_builder. Added a replace_one test for string under it's replace test, but both of those use the string_builder.replace* under the hood. Maybe that's why it's not also tested on string_builder itself?

[LagunaElectric]

@GearsDatapacks LGTM

[lpil]

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.

[LagunaElectric]

@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.

[lpil]

And you needed it for all three of these data structures in your specific case?

[LagunaElectric]

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.

[lpil]

The standard library is very conservative, we only add functions when there's real needs that benefit a good amount of people.

[LagunaElectric]

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.

[LagunaElectric]

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.