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

Add new interpolation function abs. #16168

Merged

Conversation

kwilczynski
Copy link
Contributor

@kwilczynski kwilczynski commented Sep 24, 2017

This commit adds new interpolation function abs which returns an absolute
value for a given float. Having an ability to obtain an absolute value remove
the need to use and abuse the signum function to achieve the same gaol.

Signed-off-by: Krzysztof Wilczynski kw@linux.com

@kwilczynski
Copy link
Contributor Author

Tests are passing:

$ make test TEST=./config
==> Checking that code complies with gofmt requirements...
go generate ./...
2017/09/24 23:10:48 Generated command/internal_plugin_list.go
go test -i ./config || exit 1
go list ./config | xargs -t -n4 go test  -timeout=60s -parallel=4
go test -timeout=60s -parallel=4 github.com/hashicorp/terraform/config
ok  	github.com/hashicorp/terraform/config	0.223s

@kwilczynski
Copy link
Contributor Author

Hi @vancluever, a Pull Request as discussed.

There is one thing I would love to be able to solve - math.Abs works naively on floats, and we make it work for integers here. We can specify both the ast.TypeInt and ast.TypeFloat via the ArgTypes and it would be trivial to support both. But, it seems we need to only return single type as per ReturnType, and in this case it's obviously an integer. I can't see any way to return either type or an interface{} (hoping that the underlying type would be detected, etc.), but perhaps I am wrong. Is there something that can be done to support both floats and integers?

This commit adds new interpolation function `abs` which returns an absolute
value for a given float. Having an ability to obtain an absolute value remove
the need to use and abuse the `signum` function to achieve the same gaol.

Signed-off-by: Krzysztof Wilczynski <kw@linux.com>
@kwilczynski kwilczynski force-pushed the add-abs-interpolation-function branch from 704f688 to 8a0bd9e Compare September 25, 2017 20:20
@kwilczynski
Copy link
Contributor Author

As per the conversation with @vancluever, I moved abs to use ask.TypeFloat. The only red herring here is something like this abs(-1.0), where it would return 1 rather than 1.0, which is also something that pow function "suffers" from, so to speak, and seem to be a side effect of current implementation of either parser or something internal to AST code.

Copy link
Contributor

@vancluever vancluever left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Just adding @jbardin and @apparentlymart to confirm.

Also as an aside - we uncovered some inference here that seems a bit confusing when it comes to whole numbers expressed as floats, as @kwilczynski describes above. I'm not too sure where it happens (before or after interpolation), but a whole number expressed as say, 1.0 ends up getting turned into an int somewhere, and comes out as 1, or at the very least gets displayed as an int when output.

At the very least it's kind of confusing in this context when the user wants to make sure they are getting a float back. Maybe some clarification is in order just to confirm? I'm trying to look to see exactly where this happens.

@apparentlymart
Copy link
Contributor

It's intentional that the string representation of a float representing a whole number doesn't have a fractional part. The distinction between int and float is actually planned to be removed in the future, since it doesn't really seem to be a useful distinction for things Terraform is generally used for. (more news on that to come soon.)

Generally-speaking, any time a non-list/non-map value is moved between attributes via interpolation (as opposed to within an interpolation expression) it is always converted to a string, since Terraform Core treats all primitive types as strings today. That too is something that will change in future, as part of some type system work. For now though, "getting a float back" isn't really a meaningful concept... the best you can do is get a string representation of a number, which might later get converted back into a number again when passed to other functions.

This looks fine to me, and does the best it can within the current constraints. Thanks for working on this, @kwilczynski!

@apparentlymart apparentlymart merged commit 6e7e03f into hashicorp:master Sep 25, 2017
@vancluever
Copy link
Contributor

FWIW, I think under the hood things work as expected, it seems the behaviour is mainly just superficial. Nothing that I saw would indicate anything seriously breaking as a result of this... so I think it will behave correctly 👍

@ghost
Copy link

ghost commented Apr 7, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants