feat: support copy @Deprecated #2996

chenzijia12300 · 2022-08-29T07:51:06Z

filiphr

Thanks a lot for your patience @chenzijia12300. This PR now looks really clean 😄. I've left one comment about the error, which I think that we should not be that strict.

In addition to the comment, can you please revert the changes you've done with the imports in the AdditionalAnnotationsBuilder and try to keep the same order as it was before. We do have some IDE specific settings in https://github.com/mapstruct/mapstruct/blob/main/etc that you can use to have the right formatting and import order. If you are using IntelliJ then you can import https://github.com/mapstruct/mapstruct/blob/main/etc/mapstruct.xml as a code style.

filiphr · 2022-08-29T17:34:32Z

processor/src/main/java/org/mapstruct/ap/internal/model/AdditionalAnnotationsBuilder.java

+                        annotationMirror.getAnnotationType().asElement().toString() ) ).findFirst();
+        if (optionalAnnotationMirror.isPresent() &&
+                annotations.stream().anyMatch( annotation -> annotation.getType().equals( deprecatedType ) ) ) {
+            messager.printMessage(


I think that for this particular case we don't have to add an error. We can have a warning that is saying that the method is already using @AnnotateWith(Deprecated.class) and that doing it twice is obsolete.

@filiphr , should the warning take into account the situation where the @Deprecation and the @AnnotateWith don't match?

for example:

@Deprecated( since = "1.5" ) @AnnotateWith( value = Deprecated.class, elements = { @Element( name = "forRemoval", boolean = true ), @Element( name = "since", string = "1.5.0" ) } )

Okay, so when there is a duplicate definition, a warning is raised and the type defined in @AnnotateWith(Deprecated.class) is returned

Zegveld · 2022-08-29T22:01:53Z

I'm missing the 2 annotation elements of @Deprecated: since and forRemoval.

Using a Gem for the annotation will allow you to find the values of these.
You will need to add Deprecated to the GemGenerator class so that it becomes available for usage.
Another advantage of using Gem is that you do not need to search for the deprecation annotation yourself but the generated Gem has methods that do this for you.

chenzijia12300 · 2022-08-30T03:59:18Z

Thank you for your guidance😊

Zegveld · 2022-08-30T15:41:48Z

Just looked at the reason the JDK 8 tests failed.
I totally missed this, but the since and forRemoval annotation elements are only available since java 9.
So some of the tests must be annotated that they require at least java 11, while the others also work with java 8.
@EnabledForJreRange(min = JRE.JAVA_11)
This might also require some tests to be near duplicates, one without the annotation elements for java 8+ and one with for java 11+.

chenzijia12300 · 2022-08-31T03:17:19Z

Just looked at the reason the JDK 8 tests failed. I totally missed this, but the since and forRemoval annotation elements are only available since java 9. So some of the tests must be annotated that they require at least java 11, while the others also work with java 8. @EnabledForJreRange(min = JRE.JAVA_11) This might also require some tests to be near duplicates, one without the annotation elements for java 8+ and one with for java 11+.

Thanks for the suggestion, I'm having a problem with how to exclude certain classes from compiling in a particular JDK version. I think @EnabledForJreRange only prevents the execution of the test class or test method, it does not prevent the compilation of the class.
I see a FullFeatureCompilationExclusionCliEnhancer class, please can be here for the specific class compilation exclusion?

filiphr · 2022-08-31T05:48:46Z

I think @EnabledForJreRange only prevents the execution of the test class or test method, it does not prevent the compilation of the class.

Indeed that prevents the execution of a specific test class or test method. The processors tests are not run with Java 8, they are only run with Java 11 and later.

I see a FullFeatureCompilationExclusionCliEnhancer class, please can be here for the specific class compilation exclusion?

Spot on @chenzijia12300. That class is the one that can add additional excludes for the full feature integration test. Keep in mind that you most likely will also need to add an additionalExclude in

mapstruct/integrationtest/src/test/resources/fullFeatureTest/pom.xml

Lines 22 to 28 in 68571de

    
           <properties> 
        
               <additionalExclude0>x</additionalExclude0> 
        
               <additionalExclude1>x</additionalExclude1> 
        
               <additionalExclude2>x</additionalExclude2> 
        
               <additionalExclude3>x</additionalExclude3> 
        
               <additionalExclude4>x</additionalExclude4> 
        
           </properties>

and

mapstruct/integrationtest/src/test/resources/fullFeatureTest/pom.xml

Lines 33 to 50 in 68571de

    
           <plugin> 
        
               <groupId>org.apache.maven.plugins</groupId> 
        
               <artifactId>maven-compiler-plugin</artifactId> 
        
               <configuration> 
        
                   <excludes> 
        
                       <exclude>**/erroneous/**/*.java</exclude> 
        
                       <exclude>**/*Erroneous*.java</exclude> 
        
                       <exclude>**/*Test.java</exclude> 
        
                       <exclude>**/testutil/**/*.java</exclude> 
        
                       <exclude>**/spi/**/*.java</exclude> 
        
                       <exclude>${additionalExclude0}</exclude> 
        
                       <exclude>${additionalExclude1}</exclude> 
        
                       <exclude>${additionalExclude2}</exclude> 
        
                       <exclude>${additionalExclude3}</exclude> 
        
                       <exclude>${additionalExclude4}</exclude> 
        
                   </excludes> 
        
               </configuration> 
        
           </plugin>

chenzijia12300 · 2022-08-31T06:56:48Z

Spot on @chenzijia12300. That class is the one that can add additional excludes for the full feature integration test. Keep in mind that you most likely will also need to add an additionalExclude in

Thank you for the tip 😊

filiphr

Thanks for the changes @chenzijia12300. Few more tiny changes and we are ready to merge this.

filiphr · 2022-09-02T17:21:34Z

processor/src/test/java/org/mapstruct/ap/test/annotatewith/deprecated/newer/DeprecatedTest.java

+/**
+ * @author orange add
+ */
+public class DeprecatedTest {


I see that there is some small misunderstanding of the use of the integration fullFeatureTest.

The full feature integration test does nothing more than compiling all the mappers that are in our processor test module. However, it doesn't run any tests on its own.

In addition to that, our processor tests are currently only running with Java 11 and later. Which means that we can easily move this test to the package above org.mapstruct.ap.test.annotatewith.deprecated and remove the one from org.mapstruct.ap.test.annotatewith.deprecated.older. I would also remove the @EnabledForJreRange from this tests, since it is not needed.

Can we also please change the names of the mappers a bit. e.g. everything from the older package can just be moved to the package above org.mapstruct.ap.test.annotatewith.deprecated and the prefix Older can be removed. Everything in the newer package can be moved to a different package called java11, remove the prefix Newer and do things like DeprecatedWithAttributesMapper, etc.

I am asking for this in order to reduce some of the additional test complexity and to be immediately clear when someone reads the names of the classes.

Thank you for your patient guidance, I will correct it

filiphr · 2022-09-02T17:26:23Z

processor/src/main/java/org/mapstruct/ap/internal/model/AdditionalAnnotationsBuilder.java

+            return annotations;
+        }
+        List<AnnotationElement> annotationElements = new ArrayList<>();
+        for ( Map.Entry<? extends ExecutableElement, ? extends AnnotationValue> entry : deprecatedGem.mirror()


We don't really need to use the mirror and strings for this. The DeprecatedGem should have methods that allow to have access to the values. You can rewrite the loop to:

if ( deprecatedGem.since().hasValue() ) { annotationElements.add( new AnnotationElement( AnnotationElementType.STRING, "since", Collections.singletonList( deprecatedGem.since().getValue() ) ) ); } if ( deprecatedGem.forRemoval().hasValue() ) { annotationElements.add( new AnnotationElement( AnnotationElementType.BOOLEAN, "forRemoval", Collections.singletonList( deprecatedGem.forRemoval().getValue() ) ) ); }

filiphr

LGTM thanks for all the changes @chenzijia12300. @Zegveld do you have anything extra to add or can I merge this?

filiphr · 2022-09-28T20:18:19Z

Thanks @chenzijia12300. We've integrated this into main

chenzijia12300 added 2 commits August 29, 2022 15:49

feat: support copy @deprecated

dc2f492

test: adjust unit test

b336507

filiphr requested changes Aug 29, 2022

View reviewed changes

filiphr mentioned this pull request Aug 29, 2022

feat：#2773 #2969

Closed

feat: support copy @deprecated elements and add unit test

cabebdf

test: Adapt unit tests to different JDK versions

53505b3

filiphr requested changes Sep 2, 2022

View reviewed changes

chenzijia12300 added 2 commits September 3, 2022 13:59

feat: Tuning unit tests and correct access to property values

f29a5bb

fix: Add null pointer judgment

c26dd33

filiphr approved these changes Sep 4, 2022

View reviewed changes

filiphr merged commit e979f50 into mapstruct:main Sep 28, 2022

Uh oh!

feat: support copy @Deprecated #2996

feat: support copy @Deprecated #2996

Uh oh!

Conversation

chenzijia12300 commented Aug 29, 2022 • edited by filiphr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

filiphr left a comment

Choose a reason for hiding this comment

Uh oh!

filiphr Aug 29, 2022

Choose a reason for hiding this comment

Uh oh!

Zegveld Aug 29, 2022

Choose a reason for hiding this comment

Uh oh!

chenzijia12300 Aug 30, 2022

Choose a reason for hiding this comment

Uh oh!

chenzijia12300 Aug 30, 2022

Choose a reason for hiding this comment

Uh oh!

Zegveld commented Aug 29, 2022

Uh oh!

chenzijia12300 commented Aug 30, 2022

Uh oh!

Zegveld commented Aug 30, 2022

Uh oh!

chenzijia12300 commented Aug 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

filiphr commented Aug 31, 2022

Uh oh!

chenzijia12300 commented Aug 31, 2022

Uh oh!

filiphr left a comment

Choose a reason for hiding this comment

Uh oh!

filiphr Sep 2, 2022

Choose a reason for hiding this comment

Uh oh!

chenzijia12300 Sep 3, 2022

Choose a reason for hiding this comment

Uh oh!

filiphr Sep 2, 2022

Choose a reason for hiding this comment

Uh oh!

filiphr left a comment

Choose a reason for hiding this comment

Uh oh!

filiphr commented Sep 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chenzijia12300 commented Aug 29, 2022 •

edited by filiphr

Loading

chenzijia12300 commented Aug 31, 2022 •

edited

Loading