simplify union code generator and generated union code#22
Conversation
|
|
||
| assertThat(unionTypeStringExample).isEqualTo(stringExample); | ||
| assertThat(unionTypeSet).isEqualTo(ImmutableSet.of("item")); | ||
| assertThat(unionTypeInt).isEqualTo(5); |
There was a problem hiding this comment.
so it turns out that when we added the ability to have a union over the same type this test (and the behavior more generally) became invalid
| @JsonDeserialize(builder = Union.Builder.class) | ||
| @Generated("com.palantir.conjure.java.types.BeanGenerator") | ||
| @JsonInclude(JsonInclude.Include.NON_EMPTY) | ||
| private static final class Union { |
There was a problem hiding this comment.
as a bit of an alternative for simplifying the generated code further, we could:
- drop this to just be a mutable class
- skip using the Builder class and handle everything as nullable instead of optional
| .addStatement("this.$1L = $1L", VALUE_FIELD_NAME) | ||
| private static TypeSpec generateUnionBean(TypeMapper typeMapper, UnionDefinition typeDef, String typePackage, | ||
| ClassName unionClass) { | ||
| return BeanGenerator.generateBeanTypeSpec(typeMapper, ObjectDefinition.builder() |
There was a problem hiding this comment.
could you fix the whitespace here? it makes it a little hard to read
| for (FieldDefinition def : fieldDefs) { | ||
| String getterName = BeanGenerator.asGetterName(def.getFieldName().get()); | ||
| if (first) { | ||
| block.beginControlFlow("if ($1N.$2L().isPresent())", UNION_FIELD_NAME, getterName); |
There was a problem hiding this comment.
Probably worth calling out that we now support/fix support for unions with the same type
There was a problem hiding this comment.
behavior between this code and existing code is the same, the tests I killed were invalid
|
happy to kill this/it's just a quick experiment, mostly pushed it as a thing to start a conversation around how we want to do unions. it's possible for us to cut more generated code than I've cut here at the cost of more generator code and maybe less immutable generated code. |
|
@ferozco not actually sure we wanted to merge this/probably still worth a discussion... |
|
TBH I'd prefer to only make changes like this when they are motivated by benchmarks or user requests. However, this doesn't change external facing APIs, so it's probably fine to just try it and see. Slight concerns:
|
|
cool yeah, i'm fine with throwing this away, it was a quick experiment to see if we could reduce overall amount of code, threw it up just as a discussion point |
3 participants
No description provided.