add default support in several locations #358
Conversation
modules/core/src/smithy4s/internals/DocumentEncoderSchemaVisitor.scala
Outdated
Show resolved
Hide resolved
| @@ -390,6 +392,7 @@ lazy val protocol = projectMatrix | |||
| .filterNot { case (file, path) => | |||
| path.equalsIgnoreCase("META-INF/smithy/manifest") | |||
| }, | |||
| resolvers += Resolver.mavenLocal, | |||
There was a problem hiding this comment.
Will remove when changing to use official smithy 2 release
| val streamedOutput : smithy4s.StreamingSchema[StreamedBlob] = smithy4s.StreamingSchema("GetStreamedObjectOutput", StreamedBlob.schema) | ||
| val streamedOutput : smithy4s.StreamingSchema[StreamedBlob] = smithy4s.StreamingSchema("GetStreamedObjectOutput", StreamedBlob.schema.addHints(smithy.api.Default(smithy4s.Document.fromString("")))) |
There was a problem hiding this comment.
Afaik AWS automatically adds this default since the blob shape used to be boxed and now isn't, this would go away if we updated streaming.smithy to be $version 2.0
| Option(value).orElse { | ||
| Option(defaults.get(f.label)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Not sure if orElse is as efficient here as other methods
There was a problem hiding this comment.
it's not, because of the cost of the Option allocation. Also you're only leveraging defaults values on optional fields there.
It should be :
fieldsWithDefaults.foreach { case (f, default) =>
....
if (f.isRequired) {
if(default == null) {
...
} else {
default
}
}
}
There was a problem hiding this comment.
The reason I put it in the optional side is that smithy doesn't allow required fields to have default values (required and default traits are mutually exclusive). But I do get your broader point and will make the update
There was a problem hiding this comment.
@lewisjkl actually that's an interesting notion : in the smithy4s level, I think the presence of @default should translate to a required field, and in the case class, the field should not be wrapped in Option. Essentially @default means that an instance of the case class always has a value for the corresponding field, which means in the Schema, the field should be considered required.
There was a problem hiding this comment.
Ah, I see what you're saying. I will make that change 👍
|
|
||
| import smithy4s.schema.Schema._ | ||
|
|
||
| case class DefaultTest(one: Int, two: String, three: List[String]) |
There was a problem hiding this comment.
Guessing we are going to want this to be:
| case class DefaultTest(one: Int, two: String, three: List[String]) | |
| case class DefaultTest(one: Int = 1, two: String = "test", three: List[String] = List.empty) |
If so, I'll make the changes to facilitate that
There was a problem hiding this comment.
Yeah. But also we need to make sure that the ordering of the fields goes as such :
- fully required fields first
- required fields with default second
- optional fields last.
The fields are already getting sorted to ensure that optional fields come AFTER required ones. Just change the ordering there .
* Add the logic to render default parameters * Handle unwrapped parameters, add comments * Drop Hint.* import, makes code clearer * Protect ref name Co-authored-by: David Francoeur <dfrancoeur04@gmail.com>
Adding support for default values in the various codecs (document, metadata, json).
Metadata and json are still in progress as of the time of writing this description. Tests are in place, but not yet all passing.
Notice this PR is created against a snapshot version of smithy 2 that I published locally. This is because
1.21.0-rc1doesn't handle default traits properly/completely.