mapstruct / mapstruct-idea

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

add inspection for @Mapping annotation #153

Closed hduelme closed 1 year ago

hduelme commented 1 year ago

I added three inspections for @Mapping annotation.

  1. No source property defined
  2. More than one source property defined
  3. More than one default source property defined

For the second inspection I added two possible fixes.

For the third inspection I added one possible fix. Remove one default source property.

hduelme commented 1 year ago

As @thunderhook suggested, now move to default value is only available if the source property is present.

I also fixed the pipeline. JetBrains removed the mock-Jdk Files from their main branch. As a fix I switched to the 212.5712 branch, where the files still exist. I don't think this should be a permanent solution, but for now it works :)

thunderhook commented 1 year ago

I also fixed the pipeline. JetBrains removed the mock-Jdk Files from their main branch. As a fix I switched to the 212.5712 branch, where the files still exist. I don't think this should be a permanent solution, but for now it works :)

Interesting, only the Mock-JDK11 directory was removed with this commit - 1.4, 1.7, 1.8 and 1.9 are still there. Maybe that commit can help to find a solution. 🤞

But I have no idea why the build.gradle needs this anyways. Maybe @filiphr knows why.

The latest EAP failed however. Could you please take a look at that error? Thanks in advance! 💪

hduelme commented 1 year ago

As discussed previously, I added an inspection for default properties used with source or expression source property. For this I added the possible fixes to remove the default source properties.

I also limited the More than one default source property defined, to only apply if the source property is present.

thunderhook commented 1 year ago

I opened separate issues about the EAP build and the annotations.jar problem. Feel free to tackle them. 💪

I just played around with the branch and stumbled accross this. The following code shows No source property defined, which is obviously valid:

    @Mapping(target = "testName", constant = "My name")
    Target map(Source source);

Would you please be so kind and add a test for that and fix it? Thank you!

thunderhook commented 1 year ago

Damn, I think this checks are too strict.

There are additional cases, where only a target is allowed, see:

Using your branch on the Mappers in the mapstruct repository is a good way to test the inspections.

hduelme commented 1 year ago

I opened separate issues about the EAP build and the annotations.jar problem. Feel free to tackle them. 💪

I just played around with the branch and stumbled accross this. The following code shows No source property defined, which is obviously valid:

    @Mapping(target = "testName", constant = "My name")
    Target map(Source source);

Would you please be so kind and add a test for that and fix it? Thank you!

@thunderhook I added an test where the inspection should not show any error. Although I fixed the bug.

Thanks for your feedback

hduelme commented 1 year ago

Damn, I think this checks are too strict.

There are additional cases, where only a target is allowed, see:

* https://github.com/mapstruct/mapstruct/blob/c0d88f86bf4d3de7f2f13a1c8e013d4473039142/processor/src/test/java/org/mapstruct/ap/test/bugs/_2101/Issue2102IgnoreAllButMapper.java#L20

* https://github.com/mapstruct/mapstruct/blob/53a5c34ed62739feb786a6840b5fe885296f10da/processor/src/test/java/org/mapstruct/ap/test/bugs/_2125/Issue2125Mapper.java#L23

* https://github.com/mapstruct/mapstruct/blob/e0eb0f6bb85970e534ebc4a5ea99c85c23bcf8be/processor/src/test/java/org/mapstruct/ap/test/bugs/_2164/Issue2164Mapper.java#L20

* https://github.com/mapstruct/mapstruct/blob/c1fa9bd0bd1e29b01c2c7f951543788b60a4f356/processor/src/test/java/org/mapstruct/ap/test/bugs/_2537/UnmappedSourcePolicyWithImplicitSourceMapper.java#L19

* https://github.com/mapstruct/mapstruct/blob/b35126e60948a69b82c37266d58c752cbeb1fcf7/processor/src/test/java/org/mapstruct/ap/test/bugs/_855/OrderDemoMapper.java#L18

Using your branch on the Mappers in the mapstruct repository is a good way to test the inspections.

Good catch. I will try to fix this too.

hduelme commented 1 year ago

@thunderhook I was able to fix ignoreByDefault for no source property defined https://github.com/mapstruct/mapstruct/blob/c0d88f86bf4d3de7f2f13a1c8e013d4473039142/processor/src/test/java/org/mapstruct/ap/test/bugs/_2101/Issue2102IgnoreAllButMapper.java#L20

hduelme commented 1 year ago

I although fixed https://github.com/mapstruct/mapstruct/blob/b35126e60948a69b82c37266d58c752cbeb1fcf7/processor/src/test/java/org/mapstruct/ap/test/bugs/_855/OrderDemoMapper.java#L18 dependsOn used as source

hduelme commented 1 year ago

Fixed qualifiedByName used as source https://github.com/mapstruct/mapstruct/blob/53a5c34ed62739feb786a6840b5fe885296f10da/processor/src/test/java/org/mapstruct/ap/test/bugs/_2125/Issue2125Mapper.java#L23

hduelme commented 1 year ago

@thunderhook I tested all of your cases myself and looks like I fixed it. Could you confirm?

thunderhook commented 1 year ago

Thanks alot for the contribution (and your patience with the review ping-pong 😄)