functions: add Replace, and RegexpReplace from Terraform

[azr]

No description provided.

[codecov]

Codecov Report

Merging #45 into master will decrease coverage by 5.89%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #45      +/-   ##
==========================================
- Coverage   76.08%   70.19%   -5.90%     
==========================================
  Files          77       79       +2     
  Lines        6172     7096     +924     
==========================================
+ Hits         4696     4981     +285     
- Misses       1060     1672     +612     
- Partials      416      443      +27     

Impacted Files	Coverage Δ
cty/function/stdlib/string.go	40.42% <0.00%> (-50.06%)	⬇️
cty/function/stdlib/collection.go	19.55% <0.00%> (-41.86%)	⬇️
cty/convert/compare_types.go	92.85% <0.00%> (-4.77%)	⬇️
cty/function/stdlib/datetime.go	73.77% <0.00%> (-3.44%)	⬇️
cty/convert/conversion.go	82.29% <0.00%> (-1.41%)	⬇️
cty/convert/conversion_collection.go	82.71% <0.00%> (-0.22%)	⬇️
cty/convert/unify.go	81.03% <0.00%> (-0.05%)	⬇️
cty/function/stdlib/conversion.go	90.00% <0.00%> (ø)
cty/function/stdlib/string_replace.go	100.00% <0.00%> (ø)
cty/convert/mismatch_msg.go	70.87% <0.00%> (+1.94%)	⬆️
... and 1 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c282fd1...2ba8564. Read the comment docs.

[apparentlymart]

Sorry for the delay here, @azr! Thanks for working on these.

I do like the separation of plain string replace vs. regexp replace, and would like to run with it. However, I'd prefer to see the function Replace be the ReplaceAll behavior you implemented here, and to omit the "replace with count" function for now. I say this because replacing all instances seems to be the common case and replacing a specific number seems to be a more marginal thing. I'd rather omit the specific number version until there's a clear use-case for it, and have the "all" versions have the more concise name reflecting that they are (I presume) what most people will want to use.

TLDR, I'm suggesting the following:

• Remove Replace as currently implemented.
• Rename ReplaceAll to just Replace.
• Rename RegexpReplaceAll to just RegexpReplace.

What do you think? 🤔

[azr]

Hey @apparentlymart ! No worries, thanks for reviewing, I had kept the names of the go standard library, because, naming is hard !

I agree with you that from a HCL/CTY standing point it makes more sense to have a replace call replace everything. In programming people sorts of often have the performances in mind.

Changing this.

[azr]

Here we go, I also added some tests !

[apparentlymart]

(I'm sorry I didn't catch this on the initial read yesterday)

Now that we have a separate function name as a more "normal" way to select between plain string or regex-based replacement, I'd prefer to drop this special case and just expect the whole string to be a valid regular expression. That will then be consistent with the behavior of the existing Regex and RegexAll functions already in this package. I think if leading and trailing slashes are present then they should be interpreted as a literal part of the given pattern, not as special markers.

The rest of this looks great! Thanks for the updates.

[azr]

Awesome !

[azr]

Great thanks for reviewing, this is updated and I think this is good to go noew 🙂

[apparentlymart]

Thanks! 🎉

[apparentlymart]

While I was writing the changelog entry for this I noticed that the new function was called RegexpReplace while the existing similar functions were called Regex and RegexAll (without the "p").

For consistency, I made follow up commit 440c597 to rename to RegexReplace. The implementation is otherwise unchanged. Sorry I didn't catch that before!

[azr]

Ah no worries :) Thanks !!! 🚀 🚀 🚀