feat: add regex.replace(s, p, v) to builtin #5179
Conversation
boranx
force-pushed
the
add-builtin-regex-replace
branch
from
8d3c276
to
bb48782
Compare
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
boranx
force-pushed
the
add-builtin-regex-replace
branch
from
bb48782
to
bc4c3a7
Compare
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
topdown/regex.go
Outdated
| @@ -215,4 +241,5 @@ func init() { | |||
| RegisterFunctionalBuiltin4(ast.RegexTemplateMatch.Name, builtinRegexMatchTemplate) | |||
| RegisterFunctionalBuiltin3(ast.RegexFind.Name, builtinRegexFind) | |||
| RegisterFunctionalBuiltin3(ast.RegexFindAllStringSubmatch.Name, builtinRegexFindAllStringSubmatch) | |||
| RegisterFunctionalBuiltin3(ast.RegexReplace.Name, builtinRegexReplace) | |||
There was a problem hiding this comment.
This register function is deprecated, please use RegisterBuiltinFunc, and adapt the function signature accordingly... 😳 I'm sorry it's hard to avoid this trap, we should be updating all the register calls eventually...
There was a problem hiding this comment.
Indeed! I had a branch up on my fork at one point for refactoring away the older style, at least within the standard library of builtins. I'll have to see if I can revive it.
There was a problem hiding this comment.
thanks for the review! I've transformed the registration to be using RegisterBuiltinFunc(). btw (just an idea) given all this transformation is straightforward and low-hanging fruits for new comers, it might be beneficial to mark those as good-first-issue
egrep -rnw "RegisterFunctionalBuiltin2|RegisterFunctionalBuiltin3|RegisterFunctionalBuiltin4" ./ 2>/dev/null | wc -l
51| @@ -0,0 +1,14 @@ | |||
| cases: | |||
There was a problem hiding this comment.
I don’t think you’ll need to be that comprehensive here, but a single test for an entire built-in function seems a bit sparse. How does this work with eg capturing group substitutions? How are errors dealt with?
There was a problem hiding this comment.
good idea! I've covered applying the pattern to subgroups and the error case. Currently, it returns the err as similar to what happens in other regex functions. please let me know if those are enough or if we need more cases to be written
boranx
force-pushed
the
add-builtin-regex-replace
branch
from
f39c924
to
063824f
Compare
Signed-off-by: boranx <boran.seref@gmail.com>
…and migrate to RegisterBuiltinFunc Signed-off-by: boranx <boran.seref@gmail.com>
anderseknert
force-pushed
the
add-builtin-regex-replace
branch
from
063824f
to
8ce699c
Compare
|
I think I can merge considering this is approved already (using squash&merge) |
|
Thank you for implementing this! |
* feat: add regex.replace(s, p, v) to builtin Signed-off-by: boranx <boran.seref@gmail.com> Signed-off-by: Byron Lagrone <byron.lagrone@seqster.com>
Fix: #5162
though I'm not very confident in this part of the codebase, I wanted to raise a small PR to implement the proposal above. Thanks for the reviews in advance :)