tftypes: Change (AttributePath).WithElementKeyInt() method parameter to int #101
Conversation
…hElementKeyInt() method parameter to int Reference: #59
tftypes/attribute_path.go
Outdated
|
|
||
| // WithElementKeyInt64 adds an ElementKeyInt step to `a`, using `key` as the | ||
| // element's key. `a` is copied, not modified. | ||
| func (a *AttributePath) WithElementKeyInt64(key int64) *AttributePath { |
There was a problem hiding this comment.
My controversial hot take is we should leave this out for now, and wait for someone to need it.
There was a problem hiding this comment.
I'm also completely fine with that. Wasn't sure the boundaries of fully offering "protocol support" (64 bit), but seems pragmatic to drop this here for practical reasons.
There was a problem hiding this comment.
I'm generally in the "let's make sure we can support everything the protocol can" camp for this, but I imagine if we have 2 billion elements in a list, something else is going to break first. cty also uses an int, so it would have problems too. So I'm grudgingly willing to bend to pragmatism on this one. (I also think Go will have some trouble with those values, because I don't know that slices can have more values than ints can hold.)
There was a problem hiding this comment.
Yeah, the Go spec doesn't support Array or Slice types above int: https://golang.org/ref/spec#Array_types
The length is part of the array's type; it must evaluate to a non-negative constant representable by a value of type int.
It would require a different implementation to always support 64 bit "slices" on non 64 bit systems.
.changelog/101.txt
Outdated
| tftypes: `(AttributePath).WithElementKeyInt()` now has an `int` parameter instead of `int64`. | ||
| ``` | ||
|
|
||
| ```release-note:note |
There was a problem hiding this comment.
Do we need a second note for this, in addition to the breaking change entry?
There was a problem hiding this comment.
Thought it might be nice to explain why the change was made in the changelog, but can remove or just include in the breaking change entry.
bflad
changed the title
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Closes #59