fixed Contains stdlib function #46
Conversation
Codecov Report
@@ Coverage Diff @@
## master #46 +/- ##
==========================================
+ Coverage 69.92% 70.06% +0.13%
==========================================
Files 78 78
Lines 6987 7001 +14
==========================================
+ Hits 4886 4905 +19
+ Misses 1668 1659 -9
- Partials 433 437 +4
Continue to review full report at Codecov.
|
cty/function/stdlib/collection.go
Outdated
| } | ||
|
|
||
| if args[0].LengthInt() == 0 { | ||
| return cty.NilVal, errors.New("cannot search an empty list or set") |
There was a problem hiding this comment.
I'm not sure what the equivalent Terraform function was doing here, but it feels more useful to me for this case to return false so that callers don't have to treat an empty collection as a special case. 🤔
I'd prefer to do that here in cty even if that does mean that Terraform needs to keep its own separate implementation for now to retain the previous behavior.
There was a problem hiding this comment.
Yeah, this was specifically a carry-over from terraform. I think we can even accept this directly in terraform, since there's no working case that would be relying on the error.
There was a problem hiding this comment.
It looks like this check was only ever in Terraform because it was using the Index function internally, so by writing out the implementation directly here we can "fix the glitch"! 🙃
cty/function/stdlib/collection.go
Outdated
| _, v := it.Element() | ||
| // if we don't find a match, but have an unknown element, we need | ||
| // to return unknown | ||
| if !v.IsKnown() { |
There was a problem hiding this comment.
This special case doesn't really hurt, and I'm okay with keeping it, but just wanted to note that eq := args[1].Equals(v) followed by eq.IsKnown() would've been sufficient to cach this, because Equals is an "operation method" rather than an "integration method", as defined in the cty concepts doc.
There was a problem hiding this comment.
Thanks! I will clean this up since I want to make the change above too.
Contains was transferred from Terraform, but the Index function which it was calling has the inverse meaning than the Terraform function.
Contains was transferred from Terraform, but the Index function which it
was calling has the inverse meaning than the Terraform function.