Default handling #1315
Default handling #1315
Conversation
modules/http4s/test/src/smithy4s/http4s/NullsAndDefaultEncodingSuite.scala
Outdated
Show resolved
Hide resolved
modules/http4s/test/src/smithy4s/http4s/NullsAndDefaultEncodingSuite.scala
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| test("document encoder - all default") { |
There was a problem hiding this comment.
Question: Do we need a flag to control the Document.Encoder behavior?
There was a problem hiding this comment.
I think this will be a breaking change, it requires changing DocumentEncoderSchemaVisitor, which is public.
There was a problem hiding this comment.
Considering it's in internals, we deem it's okay to break. If you're a third party who depends on what's in an internals package, it's your funeral.
Moreover, the reason we've not been ultra strict wrt private/public is because of a drastic lack of tooling that would help us do so. We'd like to build such tooling (probably by means of scalafix)
But I think it'd be totally possible to retain binary compatibility by means of secondary constructors.
There was a problem hiding this comment.
ah, I didn't realize it was in internals. Alright then
modules/http4s/test/src/smithy4s/http4s/NullsAndDefaultEncodingSuite.scala
Outdated
Show resolved
Hide resolved
modules/http4s/test/src/smithy4s/http4s/NullsAndDefaultEncodingSuite.scala
Outdated
Show resolved
Hide resolved
modules/http4s/test/src/smithy4s/http4s/NullsAndDefaultEncodingSuite.scala
Outdated
Show resolved
Hide resolved
| ) { | ||
| type Compiler = CachedSchemaCompiler[Encoder] | ||
|
|
||
| def make( |
There was a problem hiding this comment.
not a fan of this. awsHeaderEncoding shouldn't be exposed to the users here.
Also, the UX I'd like would be : Encoder.withExplicitDefaultsEncoding(true). The CachedEncoderCompilerImpl class should carry the method, which implies that we need to create an public interface for this that it will implement (as CachedEncoderCompilerImpl is private).
Also this make method should be removed.
| def fromSchema[A]( | ||
| schema: Schema[A], | ||
| cache: Cache, | ||
| explicitDefaultsEncoding: Boolean |
There was a problem hiding this comment.
There is one problem with the above approach, currently, the following doesn't compile:
Json.jsoniter.withMaxArity(1).fromSchema(Schema.string, CompilationCache.nop)
This is because JsoniterCodecCompiler inherits abstract type Cache type, wheareas in CachedSchemaCompiler.Impl it is set to type Cache = CompilationCache[Aux]. So the information about cache is lost when calling methods from JsoniterCodecCompiler.
Is it a known limitation?
There was a problem hiding this comment.
It's not a bug, it's a feature 😄
If you want to compile a codec without cache, you should be able to call
Json.jsoniter.withMaxArity(1).fromSchema(Schema.string)
Otherwise, if you wanted to use a shared cache across several codec compilations, you would have to do :
val compiler = Json.jsoniter.withMaxArity(1)
val cache = compiler.createCache()
val stringCodec = compiler.fromSchema(Schema.string, cache)
| ) | ||
| } | ||
|
|
||
| test("document encoder - all default - explicit defaults encoding = true") { |
There was a problem hiding this comment.
this should go in the DocumentSpec in the bootstrapped module.
| ) | ||
| } | ||
|
|
||
| test("document encoder - default overrides") { |
There was a problem hiding this comment.
this should go in the DocumentSpec in the bootstrapped module.
|
@Baccata addressed all the comments, not sure if |
|
@kubukoz do you want to review again ? In particular, do you want more documentation ? I wouldn't mind having more of it |
I'm happy with the current state, but some docs for the Document encoding/decoding would be welcome. I think that doesn't exist at all, though, so let's make it a separate issue which will cover the basics and the config options. |
Done |
didn't notice this 😭 thanks, LGTM |
PR Checklist (not all items are relevant to all PRs)
Closes #1290