Builder classes for service and endpoint #1097
Conversation
There was a problem hiding this comment.
Thanks for kicking this off! I took your code as a baseline and implemented the endpoint transformation function in the service mapper. I'm not sure it's fully correct, we'll have to get @Baccata's take on this, but I think we're onto something.
We'll need some tests before this can be merged, there are probably also other things...
| def mapInput(f: Schema[I] => Schema[I]): Builder[I, E, O, SI, SO] = | ||
| copy(baseInput = f(baseInput)) | ||
|
|
||
| def withOutput(output: Schema[O]): Builder[I, E, O, SI, SO] = |
There was a problem hiding this comment.
question: should we allow using a schema of a different type (another O, in this case)? I'm thinking yes, because it's certainly possible.
However, when that's used in conjunction with a service builder, I think we'd be pretty limited here - not only because PolyFunction5 forbids changing type parameters, but also I don't think we can change the operation type (which would be necessary to keep things like def endpoint: (I, Endpoint) working).
There was a problem hiding this comment.
I think there could eventually be two flavours of builders :
- One for the static usecase, that respects all the types, which probably means that the only possible transformations will be hints and labels
- One that would be more useful in the context of dynamic services, where we could amend both the types involved in operations, and the list of operations
I believe this PR is only dealing with point 1 (which is totally fine). In this context, you shouldn't allow for different types.
modules/bootstrapped/test/src/smithy4s/builder/EndpointBuilderSpec.scala
Show resolved
Hide resolved
modules/bootstrapped/test/src/smithy4s/builder/EndpointBuilderSpec.scala
Outdated
Show resolved
Hide resolved
modules/bootstrapped/test/src/smithy4s/builder/EndpointBuilderSpec.scala
Outdated
Show resolved
Hide resolved
modules/bootstrapped/test/src/smithy4s/builder/EndpointBuilderSpec.scala
Outdated
Show resolved
Hide resolved
modules/bootstrapped/test/src/smithy4s/builder/EndpointBuilderSpec.scala
Outdated
Show resolved
Hide resolved
modules/bootstrapped/test/src/smithy4s/builder/EndpointBuilderSpec.scala
Show resolved
Hide resolved
modules/bootstrapped/test/src/smithy4s/builder/ServiceBuilderSpec.scala
Outdated
Show resolved
Hide resolved
modules/bootstrapped/test/src/smithy4s/builder/ServiceBuilderSpec.scala
Outdated
Show resolved
Hide resolved
| def mapEndpointEach( | ||
| mapper: PolyFunction5[Endpoint.ForOperation[Op]#e, Endpoint.ForOperation[Op]#e] | ||
| ): Builder[Alg, Op] = copy( | ||
| endpointMapper = (endpointMapper.andThen(mapper)).unsafeCacheBy( |
There was a problem hiding this comment.
thought: this transformation is kind of lazy (only happens when you call .build), perhaps we should document that in its Scaladoc.
also, perhaps we should only cache it inside build, otherwise if you call mapEndpointEach multiple times you get multiple layers of caching.
There was a problem hiding this comment.
NB : syncing with series 0.18 will remove unsafeCacheBy, but it's okay because the exposure of ordinals should remove the need to cache.
There was a problem hiding this comment.
it did, because we got rid of def endpoint. We'll defer to the default impl, unless you prefer otherwise.
…s-for-service-and-endpoint
|
@Baccata can you take another look? |
Closes #836.