add id hints onto more generated types #36
Conversation
| @@ -517,7 +521,7 @@ private[codegen] class Renderer(compilationUnit: CompilationUnit) { self => | |||
| newline, | |||
| line(s"def to(e: $name) : (String, Int) = (e.value, e.ordinal)"), | |||
| lines( | |||
| s"val schema: $Schema_[$name] = enumeration(to, valueMap, ordinalMap)", | |||
| s"val schema: $Schema_[$name] = (enumeration(to, valueMap, ordinalMap): $Schema_[$name]).withHints(hints)", | |||
There was a problem hiding this comment.
Doesn't compile without the type hint here, not sure of another way around this atm
| val underlyingSchema : smithy4s.Schema[String] = string.withHints(hints) | ||
| val schema : smithy4s.Schema[BucketName] = bijection(underlyingSchema, BucketName(_), (_ : BucketName).value) |
There was a problem hiding this comment.
Wasn't sure if this was correct or not to have the hints on underlyingSchema.. I am thinking they should actually be on the schema following the bijection?
There was a problem hiding this comment.
I think on the whole thing, yeah (not underlyingSchema)
There was a problem hiding this comment.
It shouldn't make a difference :)
There was a problem hiding this comment.
Actually,@kubukoz is correct. On the schema, we have a chance of writing interpreters that retain the information of the id of the underlying schema;
There was a problem hiding this comment.
Actually maybe not. I think it's fine as it is : the hints should be applied on the "underlyingSchema". That is because we do not use the newtypes for collections, except when they are used as hints themselves. However, we still want the underlying schema of collections to contain the hints.
There was a problem hiding this comment.
(We should really have some smoke tests for this kind of thing 😓 )
There was a problem hiding this comment.
I thought to have this on the big thing so that the implementation of the Schematic would carry it over to the wrapped schema in a Hinted or something
There was a problem hiding this comment.
I thought to have this on the big thing so that the implementation of the Schematic would carry it over to the wrapped schema in a Hinted or something
Yeah, that is not incorrect, but having it on the small thing makes more sense, because otherwise you wouldn't have the id for collection shapes.
| @@ -480,7 +480,9 @@ private[codegen] class Renderer(compilationUnit: CompilationUnit) { self => | |||
| s"case c : $caseName => $caseName.alt(c)" | |||
| } | |||
| } | |||
| .appendToLast(if (hints.nonEmpty) ".withHints(hints)" else "") | |||
| .appendToLast( | |||
| if (error) "" else ".withHints(hints)" | |||
There was a problem hiding this comment.
errors are not an actual union in smithy, so they shouldn't really receive hints
| @@ -39,12 +39,12 @@ object enumeration { | |||
| } | |||
|
|
|||
| trait Syntax { | |||
| def enumeration[H, S[x[_]] <: Schematic[x], A]( | |||
There was a problem hiding this comment.
This was un-necessary, and preventing type inference from working properly
| with errorUnion.Syntax { | ||
|
|
||
| val short: Schema[Short] = |
There was a problem hiding this comment.
the changes bellow should address the concern of not having shape ids for primitive types
This is an attempt to close #35. There are a few areas I am unsure of (will add comments below inline). Additionally, I wasn't sure if we want to be adding the hints onto primitive types within structures and such. Thoughts?