Walk, Transform, Diff, Equal, and String. #60
Conversation
paddycarver
added
enhancement
|
A lot of this code needs GoDoc strings. More tests are also needed, as are changelog entries. These things are all forthcoming before we merge this, I just wanted to get the working code up so @alexsomesan could see and play with it. |
|
Wow! Is it Christmas all over again?! This PR is just full of juicy goodies! Regardless, this is awesome. Thanks! |
|
I am anticipating a separate PR once this is merged to make AttributePathError a bit more useful. Just trying to cut scope on this PR when I can 😂 |
|
Yup! Scattered some TODOs around this to take 58 into account. :) |
|
Fixes #53. |
This turned into a massive commit. I'm really sorry. I set out to implement Walk and Transform. Walk was simple enough. Transform was implemented quickly and easily enough. And then I tried to test it. Testing Transform reasonably necessitated all the other changes you see here: * We needed to be able to Diff Values to be able to express changes between Values so we could test that Transform made the changes we wanted it to. * We needed to be able to represent AttributePaths as strings so we could have specific, useful test output. This also entailed making Values strings (AttributePaths can use Values as a step) and making Types have more specific strings (aggregate types now expose the subtypes that make up their type signature). * In doing so, we kept accidentally overwriting our AttributePaths, meaning it was time for them to stop being modified in place and start being passed by value, with the steps defensively copied to avoid accidental modification. * We needed the ability to detect whether Diffs were equal to know whether the changes we expected were the changes we actually got or not. * We needed to make Values an AttributePathStepper so we could implement Diff, which is done by Walking one Value, and then retrieving the same sub-Value from the Value we're Diffing against to check if they're the same. We already have WalkAttributePath to retrieve part of a Go type based on the AttributePath passed in, we just needed to tell it how to index into a Value type. * We needed the ability to Copy a Value type so that it was not sharing the same memory space anymore so our transformations didn't leak across tests. The result is this PR of alarming size. The good news is a lot of these new lines are tests.
Instead of using Value.Is, use Value.Type().Is. Instead of referring to the internal-only Value.typ, use the now-standard Value.Type.
paddycarver
force-pushed
the
paddy_walk_transform
branch
from
0bad7ab to
5624f47
Compare
|
Rebased after #58 and updated to use its Type() goodies. |
Add some docstrings, rename some method receivers.
There was a problem hiding this comment.
Looks great!
I've actually made use of Transform in the Kubernetes alpha provider and it works as expected.
I did try to use Diff in a few places but it errors out with incompatible types when encountering dissimilar tuples. Because TF core always parses lists as tuples, it means any two lists with distinct amount of elements will make Diff err out. I don't think that's an issue with Diff itself, but rather an unfortunate combination of factors in my use-case.
| } | ||
| return err | ||
| } | ||
| if !shouldContinue { |
There was a problem hiding this comment.
Thought experiment: I wonder if this should somehow surface to the original caller that the visitation was interrupted before a complete exploration of all nodes could be finished. A special type of error maybe?
There was a problem hiding this comment.
This seems to mirror the Go standard library walk behaviors, in the expectation that the original caller has some control over the callback? And given the callback is the one returning shouldContinue, they should know some things may be skipped? Similarly, the callback can return its own error that the caller can handle. I guess I'm not clear on when this would be used, or why.
Add tests exercising the WalkAttributePath behavior of Value. Also, fix
a bug ("bug") in WalkAttributePath where the built in implementation
for []interface{} and the implementation for Value could panic if passed
an ElementKeyInt step that is negative. It should never happen, but
let's protect against it for fun.
|
OK, finally finished adding all the tests on this. I think we're ready for final review and merge. Apologies for the monster diff. :( This kinda spiraled out of control. |
|
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. |
This turned into a massive commit. I'm really sorry.
I set out to implement Walk and Transform. Walk was simple enough.
Transform was implemented quickly and easily enough. And then I tried to
test it.
Testing Transform reasonably necessitated all the other changes you see
here:
between Values so we could test that Transform made the changes we
wanted it to.
could have specific, useful test output. This also entailed making
Values strings (AttributePaths can use Values as a step) and making
Types have more specific strings (aggregate types now expose the
subtypes that make up their type signature).
meaning it was time for them to stop being modified in place and start
being passed by value, with the steps defensively copied to avoid
accidental modification.
whether the changes we expected were the changes we actually got or
not.
Diff, which is done by Walking one Value, and then retrieving the same
sub-Value from the Value we're Diffing against to check if they're the
same. We already have WalkAttributePath to retrieve part of a Go type
based on the AttributePath passed in, we just needed to tell it how to
index into a Value type.
the same memory space anymore so our transformations didn't leak
across tests.
The result is this PR of alarming size. The good news is a lot of these
new lines are tests.