-
-
Notifications
You must be signed in to change notification settings - Fork 1k
#2788 unmapped source properties message improved for forged methods #3522
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ins information about the forged method.
filiphr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
main changes look OK, I've added some suggestions to reduce the duplication a bit.
I've left some comments about the test changes. I am not a fan of changing existing tests, why are we doing that? Why don't we write dedicated tests for this?
...t/java/org/mapstruct/ap/test/accessibility/referenced/AbstractSourceTargetMapperPrivate.java
Outdated
Show resolved
Hide resolved
...java/org/mapstruct/ap/test/accessibility/referenced/AbstractSourceTargetMapperProtected.java
Outdated
Show resolved
Hide resolved
...rc/test/java/org/mapstruct/ap/test/accessibility/referenced/ReferencedAccessibilityTest.java
Outdated
Show resolved
Hide resolved
...test/java/org/mapstruct/ap/test/accessibility/referenced/SourceTargetMapperDefaultOther.java
Outdated
Show resolved
Hide resolved
.../test/java/org/mapstruct/ap/test/accessibility/referenced/SourceTargetMapperDefaultSame.java
Outdated
Show resolved
Hide resolved
.../src/test/java/org/mapstruct/ap/test/accessibility/referenced/SourceTargetMapperPrivate.java
Outdated
Show resolved
Hide resolved
...rc/test/java/org/mapstruct/ap/test/accessibility/referenced/SourceTargetMapperProtected.java
Outdated
Show resolved
Hide resolved
processor/src/main/java/org/mapstruct/ap/internal/model/BeanMappingMethod.java
Outdated
Show resolved
Hide resolved
processor/src/main/java/org/mapstruct/ap/internal/model/BeanMappingMethod.java
Outdated
Show resolved
Hide resolved
Not changing the existing test will make it less visible what is being tested by the test. I can split it to be more precise, so that we get separate tests for source and target. This will be obvious in the classes then.
|
Took another look at the code. and I think I'll alter the tests still, but do it in another way entirely. |
|
Ideally we should not touch the existing tests and add new ones for this. Or update the ones that are really testing the error messages. The The |
Indeed the I do feel like changing |
I am not sure that I am following. What is wrong with that test? The test is properly showing that private methods are not going to be picked up by MapStruct and thus lead to the default warning. |
default settings is in my opinion not something that the test should know of. If at some point in time the default setting changes, a lot of tests need to be fixed because they assumed that default settings would never change. But it is something that can be discussed outside of this PR. |
…source properties are by default ignored.
We can agree to disagree about this. In my opinion it is totally OK for a lot of tests to fail if we change a default, this also means that users will be affected by this and it is a good signal that something is not OK. |
…ppingMethod.java Co-authored-by: Filip Hrisafov <[email protected]>
…ppingMethod.java Co-authored-by: Filip Hrisafov <[email protected]>
|
I've accepted your suggestions for the production code, and rewritten the tests to be a part of Should be good for another pass. |
Yea those are fine since those are around testing the messages we are adapting now. Will merge once CI is green |
updated the message for unmapped source properties to have the same structure as the unmapped target properties messages.
fixes #2788