Implement OptionalAttributes for Objects. #74
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Tests are fixed by #72, will rebase this on top of it once it's merged. |
This is another massive PR, sorry. I tried to separate out some of the changes, but the code kept overlapping, unfortunately. All our Types got an Equal method, so they can be compared using go-cmp. This was necessary to make the tests pass. As part of this, the Type interface was refactored to include an equals() private method, which powers the Equal and Is methods on each type. I also fixed some periods in error messages to make golint happy. Of course, with these new methods, there was enough logic to add tests to each Type, so I added some tests that exercised the equality checks. I also added, which was the main thing I was trying to do here, an OptionalAttributes property to Objects. These are attributes that _can_ be set on an object, but are not _required_ to be. Because OptionalAttributes made the NewValue logic more complicated for Objects, I wrote some tests for creating Objects with NewValue to test the logic. Finally, the new Object comparison logic was panicking during a tfprotov5 / tfprotov6 test, because the Object had a nil AttributeTypes map instead of an empty AttributeTypes map. I fixed it to use an empty AttributeTypes map. In theory, I don't think we should ever see a nil AttributeTypes map.
paddycarver
force-pushed
the
paddy_optional_attrs
branch
from
8162ddd to
a7e10ec
Compare
kmoe
approved these changes
Apr 15, 2021
paddycarver
added a commit
that referenced
this pull request
Apr 18, 2021
Fix the checks that NewValue runs to ensure that a value can be used with a Type to properly handle DynamicPseudoTypes nested in complex types. Builds on and supersedes #76. Should we make this part of the behavior for `.Is`? My gut says "no", because `Is` is supposed to be about checking whether a type is semantically similar, not that it can be used as another type. Maybe we want a `Fulfills` method, similar to Is, that does this? I don't know that this behavior needs to be exported, though... Fixes a bug in #74 that would panic for complex types containing DynamicPseudoTypes.
paddycarver
added a commit
that referenced
this pull request
Apr 20, 2021
Fix the checks that NewValue runs to ensure that a value can be used with a Type to properly handle DynamicPseudoTypes nested in complex types. Builds on and supersedes #76. Should we make this part of the behavior for `.Is`? My gut says "no", because `Is` is supposed to be about checking whether a type is semantically similar, not that it can be used as another type. Maybe we want a `Fulfills` method, similar to Is, that does this? I don't know that this behavior needs to be exported, though... Fixes a bug in #74 that would panic for complex types containing DynamicPseudoTypes.
paddycarver
added a commit
that referenced
this pull request
Apr 21, 2021
Fix the checks that NewValue runs to ensure that a value can be used with a Type to properly handle DynamicPseudoTypes nested in complex types. Builds on and supersedes #76. Should we make this part of the behavior for `.Is`? My gut says "no", because `Is` is supposed to be about checking whether a type is semantically similar, not that it can be used as another type. Maybe we want a `Fulfills` method, similar to Is, that does this? I don't know that this behavior needs to be exported, though... Fixes a bug in #74 that would panic for complex types containing DynamicPseudoTypes. Co-authored-by: Paul Tyng <[email protected]>
|
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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is another massive PR, sorry. I tried to separate out some of the
changes, but the code kept overlapping, unfortunately.
All our Types got an Equal method, so they can be compared using go-cmp.
This was necessary to make the tests pass. As part of this, the Type
interface was refactored to include an equals() private method, which
powers the Equal and Is methods on each type.
I also fixed some periods in error messages to make golint happy.
Of course, with these new methods, there was enough logic to add tests
to each Type, so I added some tests that exercised the equality checks.
I also added, which was the main thing I was trying to do here, an
OptionalAttributes property to Objects. These are attributes that can
be set on an object, but are not required to be.
Because OptionalAttributes made the NewValue logic more complicated for
Objects, I wrote some tests for creating Objects with NewValue to test
the logic.
Finally, the new Object comparison logic was panicking during a
tfprotov5 / tfprotov6 test, because the Object had a nil AttributeTypes
map instead of an empty AttributeTypes map. I fixed it to use an empty
AttributeTypes map. In theory, I don't think we should ever see a nil
AttributeTypes map.
Fixes #48.