allow passing in the MaxArity for the JsonSchemaVisitor #569
Conversation
| @@ -31,7 +31,7 @@ object Launcher extends IOApp { | |||
| val serverRes = for { | |||
| impl <- Resource.eval(ObjectServiceImpl.makeIO) | |||
| docs = smithy4s.http4s.swagger.docs[IO](ObjectService) | |||
| service <- SimpleRestJsonBuilder.routes(impl).resource | |||
| service <- SimpleRestJsonBuilder.build.routes(impl).resource | |||
There was a problem hiding this comment.
Hmm not sure I like the idea of building the builder like this. Seems like the ideal UX would be something like:
SimpleRestJsonBuilder.routes(impl).withMaxJsonArity(2048).resource
where the call to withMaxJsonArity is not necessary if you want to use the default.
We could also take the more flexible approach of:
SimpleRestJsonBuilder.routes(impl).withJsonCodec(myJsonCodecApi).resource
What do you think?
There was a problem hiding this comment.
I definitely agree,
The problem is that SimpleProtocolBuilder doesnt have access to the JsonCodecApi as its typed to CodecApi, so we cant access the existing codec to make a copy .
We can create a new instance of JsonCodecApi inside the builder , or allow a user to pass one in , which i think is more safe and deterministic
There was a problem hiding this comment.
does it even need to be typed to JsonCodecApi? Couldn't it be just CodecApi?
There was a problem hiding this comment.
@kubukoz does it make sense to allow any codec on a RestJsonBuilder ?
There was a problem hiding this comment.
hmm now that you said it out loud...
I'd say not in general. But requiring a JSON codec on SimpleProtocolBuilder is worse.
Maybe let's add the method inside SRJB instead of the generic builder, then? And allow SimpleProtocolBuilder users to override the CodecAPI as they see fit (at the very least, with a protected method so that they're forced to subclass the protocol builder)
make SimpleRestJsonBuilder case class
| @@ -19,12 +19,17 @@ package http4s | |||
|
|
|||
| import smithy4s.internals.InputOutput | |||
|
|
|||
| object SimpleRestJsonBuilder | |||
| object SimpleRestJsonBuilder extends SimpleRestJsonBuilder(1024) { | |||
| def withMaxArity(maxArity: Int): SimpleRestJsonBuilder = | |||
There was a problem hiding this comment.
the method ought to go on the class, not the object.
There was a problem hiding this comment.
I thought it a bit odd to be on the class ,being that the class has a maxArity parameter in the constructor.
There was a problem hiding this comment.
Yeah, that's the "fluent builder" pattern. Each withMethod is basically equivalent to a copy modifying just one field, but has the quality of being much more nicer wrt bincompat.
The point of having the method on the class is that we can add more methods later on if we need to amend the builder to carry more customisation.
|
Also this PR needs to be sync'd with |
| new SchemaVisitorJCodec( | ||
| maxArity = defaultMaxArity, | ||
| CompilationCache.nop[JCodec] | ||
| ) |
There was a problem hiding this comment.
What is this version currently used for? From a quick look it seems like only in tests. Maybe we can remove.
There was a problem hiding this comment.
Im not sure what you mean by version?
There was a problem hiding this comment.
Let's add some documentation showing how to increase the maxArity and what that means
good point
There was a problem hiding this comment.
Im not sure what you mean by version?
Yeah sorry that is super confusing! I meant the "Val" vs the "def" above. The val I'm not sure is used anywhere. Thought we could remove it if that is in fact the case.
I added a method build to restJsonBuilder companion to disambiguate between the apply method which has a default value.
Im trying to minimize impact on UX.
RE: #531