Update meaning of required in both codegen and runtime.
#1301
Conversation
* Update codegen to not use the "required" terminology, as in smithy it is explicitly tied to the presence of the `required` trait. * Makes sure that the `smithy.api.Required` hint is only generated when the `smithy.api#required` is applied at the Smithy level * Updates `getUnlessDefault` or `foreachUnlessDefault` to return value/ apply lambda when the `smithy.api#required` is applied. * Update default Json hint mask to have `smithy.api.Required` explicitly * Adds relevant unit-tests for Document and Json encodings
Baccata
requested review from
lewisjkl,
kubukoz and
daddykotex
and removed request for
lewisjkl and
kubukoz
| one: String = "test" | ||
| } | ||
|
|
||
| structure DefaultVariants { |
There was a problem hiding this comment.
ahh, until I saw this structure I was unsure what use case we were not covering properly but now it's clearer
a field not marked with required with a default would use required before, now it use field
| @@ -141,13 +141,37 @@ private[internals] case class StreamingField( | |||
|
|
|||
| private[internals] object Field { | |||
|
|
|||
| sealed trait Modifier { | |||
| ) { | ||
|
|
||
| val document = Document.encode( | ||
| DefaultVariants("default", "default", "default", Some("default")) |
There was a problem hiding this comment.
suggestion: named parameters would be very helpful
| @@ -584,4 +584,36 @@ class SchemaVisitorJCodecTests() extends FunSuite { | |||
| } | |||
| } | |||
|
|
|||
| test( | |||
| "fields marked with @required and @default should always be encoded" | |||
There was a problem hiding this comment.
nitpick: good candidate for a property test?
| string.required[DefaultVariants]("req", _.req), | ||
| string.required[DefaultVariants]("reqDef", _.reqDef).addHints(smithy.api.Default(smithy4s.Document.fromString("default"))), | ||
| string.field[DefaultVariants]("optDef", _.optDef).addHints(smithy.api.Default(smithy4s.Document.fromString("default"))), | ||
| string.optional[DefaultVariants]("opt", _.opt), |
There was a problem hiding this comment.
I'm not sure I fully understand why field is used here rather than optional and it is also used in other places where required was used prior. Is field only used when a default value is present and the field is not required?
There was a problem hiding this comment.
Is field only used when a default value is present and the field is not required
Pretty much.
optionalforces the type of the getter to beS => Option[A]requiredlets you doS => A, but automatically enriches the hints withsmithy.api.Requiredfieldlets you doS => A, and doesn't enrich the hints with anything
There was a problem hiding this comment.
Got it yeah that makes sense
|
This isn't going to affect Dynamic, right? edit: I suppose |
This PR aims at moving towards a more principled way of handling required and defaults. The notion of "required" is now more strictly coupled to the presence of the
smithy.api#requiredtrait.requiredtrait.smithy.api.Requiredhint is only generated when thesmithy.api#requiredis applied at the Smithy levelgetUnlessDefaultorforeachUnlessDefaultto return value/ apply lambda when thesmithy.api#requiredis applied.smithy.api.RequiredexplicitlyCloses #1289
PR Checklist (not all items are relevant to all PRs)