search

mapstruct / mapstruct-eclipse

An Eclipse plug-in for working with MapStruct
Other
13 stars 8 forks source link

#7 Separate implementtions of ProposalComputers per annotation #16

Closed sradi closed 9 years ago

sradi commented 9 years ago

I implemented auto-completion for @Mapper#componentModel. For this addition, I refactored the code to have separate proposal computer classes for each annotation. @larswetzer: What do you think about this change? Any hints to improve the code are welcome.

The list of supported component models is currently a fix list. Does anybody have a suggestion to improve this? Perhaps, if the supported values were implemented as an enum in mapstruct, I could read the supported values with reflection.

wetzer commented 9 years ago

Thanks a lot, @sradi! I'll have a look at it asap.

gunnarmorling commented 9 years ago

The list of supported component models is currently a fix list. Does anybody have a suggestion to improve this? Perhaps, if the supported values were implemented as an enum in mapstruct, I could read the supported values with reflection.

I think it's ok as is. It is intentionally not an enum in MapStruct, in order to allow for the addition of custom component models down the road. Admittedly, that'd require some more changes (such as making the templates somehow user-customizable), but that was the motivation behind it.

gunnarmorling commented 9 years ago

Hi @sradi, looks good to overall, just added some minor, mostly style-related comments. Thanks a lot for your first contribution to the plug-in!

Btw. do you think you could rework this commit and split it up into two: one for the refactoring and one for the new feature? Generally, smaller, dedicated commits are easier to grasp. Thanks again!

gunnarmorling commented 9 years ago

@sradi I took another look and I came to wonder why there is MapStructCompletionProposalComputer and the hierarchy of AbstractAnnotationCompletionProposalComputer s? Wouldn't it make more sense to just have one base class, the two implementations for @Mapper and @Mapping and register these two implementations as extensions in plugin.xml?

If you like the idea, just rework the concerned commits, otherwise I can do it as well. WDYT?

sradi commented 9 years ago

@gunnarmorling oh, sure. I will change this.

gunnarmorling commented 9 years ago

@sradi Will you send a new pull request then? You also can re-open this one and (force-) push further commits to this branch. Whatever you like better :)