Refactor target reference to report errors during bean mapping instead of creation of the target reference - #2068
Conversation
sjaakd
left a comment
sjaakd
left a comment
There was a problem hiding this comment.
Hi Filip,
I think that for the majority I'm ok.. But I do have some questions:
- The whole purpose is to delay checking of the stuff in
TargetReferenceto the moment that we doBeanMappingvalidations right? - I see you dropped
TargetReferencebut it had an abstract base classAbstractReferenc(shared withSourceReference.. is this still needed? - Do I understand you correctly and you replaced the
PropertyEntriesby aString. should we do the same at the source side?
| <!-- See http://checkstyle.sf.net/config_sizes.html --> | ||
| <module name="MethodLength"/> | ||
| <module name="MethodLength"> | ||
| <property name="max" value="200"/> |
There was a problem hiding this comment.
haha, maybe even completely remove it? I am all OK for checks and the likes, but it should not be blocking us
| message = "Unknown property \"colour\" in type org.mapstruct.ap.test.namesuggestion.Garage for target" + | ||
| " name \"garage.colour.rgb\". Did you mean \"garage.color\"?"), | ||
| @Diagnostic(type = PersonGarageWrongTargetMapper.class, | ||
| kind = javax.tools.Diagnostic.Kind.WARNING, |
There was a problem hiding this comment.
Before the entire TargetReference for garage.colour.rgb was invalid which meant that handling the defined mappings led to an hour which stopped the creating of the BeanMappingMethod and this not reporting the unmapped target.
However, with the new change the target reference does not have valid property anymore, so the defined mappings would not report this error and thus we would reach the unmapped target properties reporting.
IMO this is a good thing, since you have more warnings / errors that you can look at initially
|
Thanks a lot for the review @sjaakd. Answers to your questions inline
Yes the idea was that the do the checking of the stuff in
What do you mean I dropped? I am no longer inheriting from the
Yes that is exactly what I did replacing the property entries with a collection of |
No.. I formulated it a bit crummy.. Sorry. You removed the abstract class |
|
OK Now it is clear. Yes you are completely right. However, I'd like to keep that separate. I created #2070 to track the |
… mapping instead of creation of the target reference With this we can more easily go in the direction of using constructor to map into target beans.
e79fb45 to
b122a8e
Compare
2 participants
With this we can more easily go in the direction of using constructor to map into target beans.
This is a step towards #73
In this PR all tests are passing green (had to change some, but I think that this is OK).
There is an alternative approach to this. That would be to have dedicated
ForgedMappingReferencesfrom which we would be able to create the newMappingReferenceslike we are doing for theSourceMethod. Basically the problem is that we first need to get the accessors and result type and then construct theTargetReferenceFixes #2069