Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sprintf: fix number formatting quirks wrt float/int #4610

Open
srenatus opened this issue Apr 22, 2022 · 8 comments · Fixed by #4620
Open

sprintf: fix number formatting quirks wrt float/int #4610

srenatus opened this issue Apr 22, 2022 · 8 comments · Fixed by #4620
Assignees
Labels

Comments

@srenatus
Copy link
Contributor

$ opa eval -fpretty 'sprintf("%f", [13])'
"%!f(int=13)"

This should output "13". Using %v is a workaround, but won't allow us to control the number of digits after the dot, like %.3f would allow.

The problem is this code: https://github.com/open-policy-agent/opa/blob/v0.39.0/topdown/strings.go#L427-L436

☝️ We're trying to find the best format of the arg to pass along to the golang stdlib Printf function. The heuristic breaks down when you try to format a number that could be an int or a float internally.

Here's another example:

$ opa eval -fpretty --import future.keywords '{ sprintf("%f", [x]) | some x in {1, 1.1}}'
[
  "%!f(int=1)",
  "1.100000"
]

The desired output would be [ "1", "1.100000"].

💭 One approach would be to examine the format string, and prepare the number argument accordingly: If the n-th format specifier is of the float-y type, make the n-th arg a float; if it's int-y, make it an int.

@srenatus srenatus added the bug label Apr 22, 2022
@damienjburks
Copy link
Contributor

damienjburks commented Apr 22, 2022

I'd like to take a stab at completing this! Please assign it to me.

@srenatus
Copy link
Contributor Author

Thanks! LMK if you need assistance or if something is unclear or if you'd like to discuss some approach... 😃

@damienjburks
Copy link
Contributor

Question: @srenatus Let's say I set the format to %.3f in the example you've shown above. Would this be the expected output?

$ opa eval -fpretty --import future.keywords '{ sprintf("%.3f", [x]) | some x in {1, 1.1}}'
[
  "001",
  "1.100"
]

OR would you prefer to see the exact same with the int value?

$ opa eval -fpretty --import future.keywords '{ sprintf("%.3f", [x]) | some x in {1, 1.1}}'
[
  "1",
  "1.100"
]

@srenatus
Copy link
Contributor Author

Hmm I think I'd go with "1.000". Like here, https://go.dev/play/p/tNR1eM9TpSD

alikhajeh1 added a commit to infracost/gh-actions-demo that referenced this issue May 27, 2022
alikhajeh1 added a commit to infracost/gh-actions-demo that referenced this issue May 27, 2022
alikhajeh1 added a commit to infracost/gh-actions-demo that referenced this issue May 27, 2022
* Increase IOPS

* Remove policy-path until open-policy-agent/opa#4610 has been fixed
srenatus pushed a commit that referenced this issue Jun 7, 2022
Fixes #4610.

Signed-off-by: Damien Burks <damien@damienjburks.com>
alikhajeh1 added a commit to infracost/gh-actions-demo that referenced this issue Jul 7, 2022
feat: add this back as open-policy-agent/opa#4610 has been fixed
@hugorut
Copy link

hugorut commented Jul 21, 2022

Hi there, should we reopen this issue since the fix has been reverted? #4794

Or is there another issue I should watch to track a fix for this behaviour?


Edit: maybe #4771 is what I'm missing, apologies

@srenatus
Copy link
Contributor Author

Yeah that other issue is what to watch now. We could reopen this one, anyways, you're right.

@srenatus srenatus reopened this Jul 21, 2022
@hooksie1
Copy link

hooksie1 commented Mar 4, 2024

Is there any update on this? Just got bit by this recently.

@srenatus
Copy link
Contributor Author

srenatus commented Mar 4, 2024

Nope, no work has happened on this since, AFAICT 😓

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Backlog
Development

Successfully merging a pull request may close this issue.

4 participants