Conversation
| recordTypeVariables = toTypeVariableNames(record.getTypeParameters()); | ||
| List<TypeVariableName> builderTypeParams = new ArrayList<>(recordTypeVariables); | ||
| List<TypeVariableName> builderTypeParamsWildcard = new ArrayList<>(recordTypeVariables); | ||
| var self = TypeVariableName.get("SELF"); |
There was a problem hiding this comment.
SELF should be defined in RecordBuilder.Options metadata in case some client code already uses this name. The default would be better to be something very unique. Maybe RECORD_BUILDER_SELF.
|
|
||
| String builderName = getBuilderName(record, metaData, recordClassType, metaData.suffix()); | ||
| recordTypeVariables = toTypeVariableNames(record.getTypeParameters()); | ||
| List<TypeVariableName> builderTypeParams = new ArrayList<>(recordTypeVariables); |
There was a problem hiding this comment.
The construction of builderTypeParams and builderTypeParamsWildcard shouldn't result in a modifiable ArrayList. Please move to separate method or ElementUtils so that the constructed type is immutable.
There was a problem hiding this comment.
A general comment. Every generated sub-class should have the same Parameterized Type list as the builder but this PR doesn't do that. It looks like builderTypeParamsWildcard does that instead? Why not do what was done before: i.e. all the nested Stage/With classes should have the exact same Parameterized Type list as the builder.
| class TestParameterizedBuilder { | ||
| @Test | ||
| void testNoSubclass() { | ||
| ParameterizedBuilderBuilder<?> builder = ParameterizedBuilderBuilder.builder(); |
There was a problem hiding this comment.
It feels odd to have a static builder in a parameterized builder. It should likely not be added for parameterized and force use of the constructors.
There was a problem hiding this comment.
Is the no-subclass even useful? We could make the builder abstract when parameterized.
2 participants
#157 (comment)