#163 Reference support for @InheritConfiguration and @InheritInverseConfiguration #199
Conversation
…tInverseConfiguration
src/main/java/org/mapstruct/intellij/codeinsight/references/MapstructBaseReference.java
Show resolved
Hide resolved
| * | ||
| * @author Oliver Erhart | ||
| */ | ||
| public abstract class MapstructNonNestedBaseReference extends MapstructBaseReference { |
There was a problem hiding this comment.
I tried to provide this kind of base reference on the level of MapstructBaseReference but it would have led to more duplication. Therefore I decided to provide a base reference on this level.
There was a problem hiding this comment.
I'm not sure that I understand what MapStructNonNestedBaseReference means.
There was a problem hiding this comment.
It is a reference that does not support nesting, see:
This is called with true from source or target references:
Then it is a reference that supports the nested, dotted notation like property.nested.field. So this is possible if the reference is about a type, I guess?
It is false, when nesting is not supported, so in MapstructMappingInheritConfigurationReference, MapstructMappingInheritInverseConfigurationReference, MapstructMappingQualifiedByNameReference where the references are not nested, like method names. And this base class is for those candidates.
Got a better naming idea? I also called it MapstructUntypedBaseReference, but that was even worse.
There was a problem hiding this comment.
It feels to me like we need a type like MapStructPropertyReference which would be for the source and target references (which are the only ones where we have nested properties. I wouldn't mix the nested word though in the types, it's a bit strange to me
src/main/java/org/mapstruct/intellij/inspection/inheritance/SourceMethod.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Nice work @thunderhook.
I had a look at it, and I'm not sure that I like the new things in mergeInheritedOptions. It feels like we are doing multiple things in the same time, when we actually only need one thing. Perhaps we need to refactor that to only get what we actually need instead of trying to fit everything into the InheritConfigurationContext. If feels more like we need methods that would return exactly what we need instead of having mappedTargets, forwardTemplateCandidates and inverseTemplateCandidate in one place when most of them aren't needed anyways.
| @Mapping(target = "auditTrail", constant = "fixed") | ||
| abstract CarEntity toCarEntityWithFixedAuditTrail(CarDto carDto); | ||
|
|
||
| // this method should not be considered. See issue #1013 |
There was a problem hiding this comment.
The referenced issues is from MapStruct, right? It is mapstruct/mapstruct#1013? Perhaps we need to put that link directly here.
There was a problem hiding this comment.
Yes, good catch, used the example from the mapstruct repository and that was copy/pasted.
| * | ||
| * @author Oliver Erhart | ||
| */ | ||
| public abstract class MapstructNonNestedBaseReference extends MapstructBaseReference { |
There was a problem hiding this comment.
I'm not sure that I understand what MapStructNonNestedBaseReference means.
I feel you. I would have loved to implement it this way. However, the current logic has been extracted (and adapted) from the original MapStruct source ( For me, at least, I think it is better not to have a completion than to have a possible wrong one that leads to frustration. I think if the inheritance logic is to be refactored, it should be done in the MapStruct source first, as there is a lot more code coverage there. But even then, I personally would be too scared to touch it. Maybe it is because of a performance optimisation by not calculating things multiple times. WDYT? Should I try to rewrite it? Maybe I can find some time in July/August. Not sure why my comments are not listed directly under your comments, sorry for the confusion. |
I understand what you are saying. However, I think that there is a big difference in the possible optimizations for the annotation processor vs the IntelliJ plugin. The plugin runs in the scope of the user view and at the time the file is displayed to the user. We should try to be as optimal as possible there, we do not want to make IntelliJ sluggish. If you want I can look into refactoring those bits? |
Yes, you're welcome to do that. I'm curious to see what it looks like after the refactoring. 👍 |
Resolves #163
Provided two test cases with multiple completion candidates.