#159 support fluent setters with super types (e.g. lomboks @SuperBuilder) by thunderhook · Pull Request #161

#159 support fluent setters with super types (e.g. lomboks @SuperBuilder) by thunderhook · Pull Request #161 · mapstruct/mapstruct-idea

Conversation

I hope this change has no unwanted side effects. It checks whether a fluent setters return type also can be assigned to one of its super types to support classes like this:

public static abstract class TargetBuilder<C extends Target, B extends TargetBuilder<C, B>> {

            private String testName;

            public B testName(String testName) {
                this.testName = testName;
                return self();
            }
            // ...
}

Fixes #159

Check my comment in #159 (comment). Is the test code working even properly with the MapStruct processor. We don't really support @SuperBuilder from Lombok, so the fix here will be a bit misleading.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good @thunderhook, I've added some small comments

Comment on lines +224 to +225

		return Arrays.stream( returnType.getSuperTypes() )
		.anyMatch( superType -> isAssignableFrom( psiType, superType ) );

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no need to create a stream and then use anyMatch. Let's just iterate through the returnType.getSuperTypes() and do a check in the for loop.


		private static boolean isAssignableFromReturnTypeOrSuperTypes(PsiType psiType, @Nullable PsiType returnType) {

		if ( returnType == null ) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The returnType cannot be null because in isFluentSetter we are checking method.getReturnType() != null.

@filiphr Thanks for the feedback. I have made the requested changes.

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

2 participants