feat: Add support for flexible defaults encoding #1652
Conversation
| def compileOptional[A]( | ||
| field: Field[?, A] | ||
| ): FieldRenderPredicateCompiler.ShouldSkip[A] |
There was a problem hiding this comment.
thought: I was tempted to cast Field[?,A] to Field[?, Optional[A]] in OptionalPredicate but that doesn't work for fields with a default value.
There was a problem hiding this comment.
Let's call it SkipNonRequired instead. The term "optional" is more related to our Scala-specific interpretation, which differs to the absence of @required (for instance when default values are involve, as you point out)
| wrappedCollection => | ||
| wrappedCollection.asInstanceOf[Option[Any]].exists(predicate) | ||
| ) | ||
| case LazySchema(_) => None // ? |
There was a problem hiding this comment.
Not sure what to do about that LazySchema. Should I simply write:
case LazySchema(inner) => asEmptyCollectionPredicate(inner.value)?
There was a problem hiding this comment.
I think so. You'll have to add a test to ensure it doesn't recurse infinitely
| wrappedCollection.asInstanceOf[Option[Any]].exists(predicate) | ||
| ) | ||
| case LazySchema(_) => None // ? | ||
| case _: MapSchema[k, v] => None |
There was a problem hiding this comment.
question: should maps be also taken care of here?
| import smithy4s.schema.Field | ||
| import smithy4s.schema.Schema | ||
|
|
||
| trait FieldRenderPredicateCompiler { self => |
There was a problem hiding this comment.
question: should this be sealed?
There was a problem hiding this comment.
No, users should be able to create their own.
| def this(cache: CompilationCache[DocumentEncoder]) = | ||
| this(cache, explicitDefaultsEncoding = false) | ||
|
|
||
| protected val explicitDefaultsEncoding: Boolean = |
There was a problem hiding this comment.
thought: kept for binary compatibility reasons
There was a problem hiding this comment.
add a @deprecated onto it.
|
I am not happy about the implementation of |
| import smithy4s.schema.Field | ||
| import smithy4s.schema.Schema | ||
|
|
||
| trait FieldRenderPredicateCompiler { self => |
| import smithy4s.schema.Schema | ||
|
|
||
| trait FieldRenderPredicateCompiler { self => | ||
| def compile[A]( |
There was a problem hiding this comment.
let's change the interface to
def compile(label: String, schema: Schema[A]) : A => Boolean
final def compileField[S, A](field: Field[S, A]) : A => Boolean = compile(field.label, field.schema)
| field: Field[?, A] | ||
| ): FieldRenderPredicateCompiler.ShouldSkip[A] | ||
|
|
||
| def combine( |
There was a problem hiding this comment.
let's make two combinators : && and ||. The behaviour will be clearer to end-users
|
|
||
| type ShouldSkip[A] = A => Boolean | ||
|
|
||
| trait OptionalPredicate extends FieldRenderPredicateCompiler { |
| } | ||
|
|
||
| private def asEmptyCollectionPredicate[F[_]]( | ||
| schema: Schema[_] |
There was a problem hiding this comment.
avoid using existential types as parameters : make it :
private def emptyCollectionPredicate[F[_], A](schema: Schema[A]) : Option[A => Boolean]
| val SkipIfEmptyOrDefaultOptionals = | ||
| SkipIfEmptyOptionals combine SkipIfDefaultOptionals | ||
|
|
||
| def fromExplicitDefaults( |
There was a problem hiding this comment.
remove this, just inline the code where it's used
| } | ||
| } | ||
|
|
||
| val SkipIfEmptyOrDefaultOptionals = |
There was a problem hiding this comment.
I'd rather we removed all the ones that are mere combinations, and let the users combine them themselves
|
A good start 👍 |
|
@kubukoz wanna have a look? |
PR Checklist (not all items are relevant to all PRs)
Added bootstrapped code + smoke tests (when the rendering logic is modified)N/AAdded build-plugins integration tests (when reflection loading is required at codegen-time)N/AAdded alloy compliance tests (when simpleRestJson protocol behaviour is expanded/updated)N/AUpdated dynamic module to match generated-code behaviourN/ARefers to #1398