Skip to content

#598 superclass mapping methods now throw errors/warnings on mapper class.#2608

Closed
Zegveld wants to merge 0 commit into
mapstruct:masterfrom
Zegveld:feature/598
Closed

#598 superclass mapping methods now throw errors/warnings on mapper class.#2608
Zegveld wants to merge 0 commit into
mapstruct:masterfrom
Zegveld:feature/598

Conversation

@Zegveld

@Zegveld Zegveld commented Oct 10, 2021

Copy link
Copy Markdown
Contributor

Here is the first (and maybe final) draft of the PR for issue #598. I've made adjustments to the FormattingMessager interface, so this PR will most likely have impact on other PR's that introduce error/warning/info messages during compilation.

FormattingMessager.printMessage now expects 2 elements instead of 1 (those without elements are unaffected):

  1. The mapper element (can be null)
  2. The element on which the error triggered (may not be null)

@filiphr

filiphr commented Oct 16, 2021

Copy link
Copy Markdown
Member

Thanks for the PR @Zegveld.

I like the idea. However, I am not to happy with the fact that we have to expose this on the FormattingMessager itself. I would propose an alternative solution. Instead of doing this on the caller side we could do it by creating a new instance of the FormattingMessage. This can be done in the MapperCreationProcessor by creating a new instance using the mapperTypeElement which is actually the @Mapper for which an implementation is being created at the moment and I guess is the one in the sourceMethod for you.

Regarding

when trying to add the message I get different results depending on the compiler:
jdk: Occured at 'map(SOURCE)' in 'AbstractMapper'.
eclipse: Occured at 'public abstract TARGET map(SOURCE) ' in 'AbstractMapper'.

Instead of testing it like that I would rather cast to Executable element (if the element is a method) and provide a consistent message from our side.

@Zegveld

Zegveld commented Oct 19, 2021

Copy link
Copy Markdown
Contributor Author

I agree with that the current solutions isn't the best one, I've been thinking about another way for a while including what you mentioned but so far I haven't been able to get that route working without duplicating a lot of code. Seeing as I'm not a fan of code duplication I'm not willing to put that solution forward.

My new idea is to use a decorator pattern on the Message object as well as on the FormattingMessager.
This would basically mean that the Message object gets an interface which will be implemented by both the current enum, and the decorator. The decorator is constructed by putting an object matching the interface in it, and most of the functionality will be passed through, except for the bits that we want to adjust. These bits are the message location (Edit: at the FormattingMessager) and an extension to the message text (Edit: at the Message).

Originally I tried to use the decorator pattern purely on the FormattingMessager itself, but that didn't work out that well. But if I combine a decorator on the FormattingMessager with a decorator on the Message object, then I should be able to get it working nicely. Also this will allow a bit more flexibility with messages enhancement in the future.

Going to work on this idea during the coming weekend, if I have time.

(Edit: This PR got automatically closed because I forced the changes away. 😸 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants