#1742 refactoring and making builder optional#1811
Conversation
…type always available
filiphr
left a comment
filiphr
left a comment
There was a problem hiding this comment.
Wow @sjaakd this is an amazing PR 😄.
All your points are on point.
Regarding the SourceReference and TargetReference they are already used mostly within a BeanMappingMethod context. They are derived from the @Mapping that is mostly used for bean mapping methods. Or am I missing something? In any case you are completely right about the separation of the concerns.
I experimented with making annotations and annotation fields
Optional. I think we should apply this pattern, because we keep on doing null checks all over the place.
IMP doing a not null check vs Optional#isPresent there is not a big difference.
I've added some comments to the PR. My "biggest" concern is the naming of the noBuilder. For some reason it doesn't stick with me. How about disableBuilder? Maybe something else, I am open to suggestions.
|
I forgot to mention. This PR would also solve #1661, right? |
it does.. |
|
For #1661 let's also add an annotation processor option. So this would partially solve it. |
|
Thanks for the updates. LGTM for merging |
|
I just had a quick glance at your changes, looks awesome. Thanks @sjaakd ! |
3 participants
Currently the accessor type is defined in PropertyMapper while it should be an intrinsic property of Accessor. Types are fetched and derived in context (again) all-over-the-place.. Sometimes that requires the parent type again and even the mapping method (to derive in context and resolve generics).
Next, builderType is part of type. One could say there is a relation from builderType to Type but not the other way around. A builderType could be in a completely different package. It's still not as I want.. But I managed to move the builderType (mostly) to the place were its relevant: the
BeanMappingMethod.Done!
I'm still not completely happy with the number of places were I do need to consult the typeFactory for the
effectiveTypeor thebuilderType. This indicates a bad separation of concerns. Probably stemming from the fact that we do analyse a nested target and try to find the associated types inTargetReference. I think that in the endTargetReferenceandSourceReferenceshould be handled in theBeanMappingMethodand not inMappingupfront but in the context of theBeanMappingMethod.I experimented with making annotations and annotation fields
Optional. I think we should apply this pattern, because we keep on doing null checks all over the place.Fixes #1742