sprintf builtin - #43
Conversation
Sorry, something went wrong.
anderseknert
left a comment
There was a problem hiding this comment.
There aren't that many tests for sprintf I'm afraid, as the OPA implementation simply mirrors the Go one. This was a mistake that has been problematic for all alternative implementations, as some things that Go's sprintf allows isn't feasible to implement in other languages. So the best thing is likely just to provide a best-effort implementation and to make it very clear that for anything non-trivial, the result is likely going to differ or not work at all.
The code looks good to me, but if you have somewhere where you've documented the supported built-is, it would be good to add a note about this there.
Sorry, something went wrong.
I see that it's already marked as "SDK-dependent": do you think we should do anything more?
right, done, thanks for the reminder! |
Sorry, something went wrong.
| At the moment the following builtins are supported(and, by default, automatically injected when needed): | ||
|
|
||
| - String | ||
| - `sprintf` |
There was a problem hiding this comment.
Good! What I alluded to before is that it would be really good to mention here how this built-in won't behave exactly as sprintf in OPA, and should ideally only be used for simple operations, like %s, %d and such.
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks for the feedback, I added a note, let me know how it reads 🙏
Sorry, something went wrong.
anderseknert
left a comment
There was a problem hiding this comment.
Great! Thanks 👍
Sorry, something went wrong.
Implement the sprintf String builtin.
@anderseknert I was unable to find testcases to cover the edge cases, do you have a link to spare?