fix: remove invalid defaults for some services #1112
Closed
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
milesziemer
force-pushed
the
remove-defaults-for-invalid-range-traits
branch
2 times, most recently
from
6c5d483 to
dacde52
Compare
ianbotsf
approved these changes
Nov 17, 2023
Comment on lines
37
to
65
| override fun enabledForService(model: Model, settings: KotlinSettings): Boolean { | ||
| val serviceId = settings.service | ||
| return serviceId in removeDefaultsFrom | ||
| } |
There was a problem hiding this comment.
Nit: Can be simplified:
override fun enabledForService(model: Model, settings: KotlinSettings) =
settings.service in removeDefaultsFrom
Comment on lines
69
to
73
| private fun shouldRemoveRootDefault(shape: Shape, removeDefaultsFrom: Set<ShapeId>): Boolean = | ||
| shape !is MemberShape && removeDefaultsFrom.contains(shape.id) && shape.hasTrait<DefaultTrait>() | ||
|
|
||
| private fun shouldRemoveMemberDefault(shape: Shape, removeDefaultsFrom: Set<ShapeId>): Boolean = | ||
| shape is MemberShape && removeDefaultsFrom.contains(shape.target) && shape.hasTrait<DefaultTrait>() |
There was a problem hiding this comment.
Style:
removeDefaultsFrom.contains(shape.id)→shape.id in removeDefaultsFromremoveDefaultsFrom.contains(shape.target)→shape.target in removeDefaultsFrom
aajtodd
approved these changes
Nov 17, 2023
| import software.amazon.smithy.model.traits.DefaultTrait | ||
| import kotlin.test.assertFalse | ||
|
|
||
| class RemoveDefaultsTest { |
There was a problem hiding this comment.
suggestion: This test could be improved by verifying it doesn't remove defaults from shapes not in the set and that it also removes them from member shapes.
milesziemer
force-pushed
the
remove-defaults-for-invalid-range-traits
branch
3 times, most recently
from
ad40494 to
21e392c
Compare
Closed
Adds an integration that removes the default value from certain shapes in some services where the default value is 0, but the service expects a value > 0.
milesziemer
force-pushed
the
remove-defaults-for-invalid-range-traits
branch
from
21e392c to
6aebd1c
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
ianbotsf
approved these changes
Nov 17, 2023
|
closing in favor of #1116 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Issue #
N/A
Description of changes
Adds a customization that removes default values from certain shapes for some services. These default values could cause
issues if the service expects a value > 0. Removing the default means these values aren't set under the hood causing unexpected request failures.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.