search

mapstruct / mapstruct-idea

An IntelliJ IDEA plugin for working with MapStruct
Other
136 stars 38 forks source link

#163 Reference support for @InheritConfiguration and @InheritInverseConfiguration #199

Open thunderhook opened 3 months ago

thunderhook commented 3 months ago

Resolves #163

Provided two test cases with multiple completion candidates.

thunderhook commented 3 months ago

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.

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 (MapperCreationProcessor). This is how the inheritance is evaluated there. Writing a new implementation could lead to false positives.

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.

filiphr commented 1 month ago

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 (MapperCreationProcessor). This is how the inheritance is evaluated there. Writing a new implementation could lead to false positives.

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?

thunderhook commented 1 month ago

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. 👍