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

@InheritInverseConfiguration cannot directly inherit from prototype methods #1065

Closed
sjaakd opened this Issue Feb 5, 2017 · 5 comments

Comments

Projects
None yet
4 participants
@sjaakd
Contributor

sjaakd commented Feb 5, 2017

I have the simple use case:

  1. SharedConfig
  2. Reuse SharedConfig to separate from-to, to-from into 2 separate classes, define the mappings once.

It does not work..

Applied to our unit test: org.mapstruct.ap.test.inheritfromconfig.InheritFromConfigTest, this is the reproducer:

@Mapper(
    uses = NotToBeUsedMapper.class,
    config = AutoInheritedConfig.class,
    mappingInheritanceStrategy = MappingInheritanceStrategy.EXPLICIT
)
public abstract class CarMapperReverseWithExplicitInheritance {
    public static final CarMapperReverseWithExplicitInheritance INSTANCE =
        Mappers.getMapper( CarMapperReverseWithExplicitInheritance.class );

    @InheritInverseConfiguration(name = "baseDtoToEntity")
    public abstract CarDto toCarDto(CarEntity entity);

}

Note: a similar method is there in CarMapperWithExplicitInheritance, but there the reverse mapping runs via a local present method.

@sjaakd sjaakd self-assigned this Feb 5, 2017

@sjaakd sjaakd added the bug label Feb 5, 2017

@sjaakd sjaakd added this to the 1.2.x milestone Feb 5, 2017

@sjaakd

This comment has been minimized.

Show comment
Hide comment
@sjaakd

sjaakd Feb 5, 2017

Contributor

@agudian , @gunnarmorling : do you guys remember why I can't inherit inverse mappings from a SharedConfig. The use case sketched above seems to justify such mappings. In my head, these @InheritConfiguration and @InheritInverseConfiguration were more or less symmetric. Yet the code seems to be designed with only @InheritConfiguration in mind when it comes to prototype methods..

Contributor

sjaakd commented Feb 5, 2017

@agudian , @gunnarmorling : do you guys remember why I can't inherit inverse mappings from a SharedConfig. The use case sketched above seems to justify such mappings. In my head, these @InheritConfiguration and @InheritInverseConfiguration were more or less symmetric. Yet the code seems to be designed with only @InheritConfiguration in mind when it comes to prototype methods..

@agudian

This comment has been minimized.

Show comment
Hide comment
@agudian

agudian Feb 20, 2017

Member

I don't think there was a specific reason for not implementing it -- I just didn't think of it, I guess. At least I can't remember anything else... 😁

Perhaps it can be fixed when the stuff get's refactored for making them applicable in the forged methods?

Member

agudian commented Feb 20, 2017

I don't think there was a specific reason for not implementing it -- I just didn't think of it, I guess. At least I can't remember anything else... 😁

Perhaps it can be fixed when the stuff get's refactored for making them applicable in the forged methods?

@sjaakd

This comment has been minimized.

Show comment
Hide comment
@sjaakd

sjaakd Feb 20, 2017

Contributor

Good to know.. I've got a PR in the making.. However UT's start to fail (because they refer to local reverse methods)..

Contributor

sjaakd commented Feb 20, 2017

Good to know.. I've got a PR in the making.. However UT's start to fail (because they refer to local reverse methods)..

@gunnarmorling

This comment has been minimized.

Show comment
Hide comment
@gunnarmorling
Member

gunnarmorling commented Feb 23, 2017

Done.

@filiphr filiphr modified the milestones: 1.2.0.Beta2, 1.2.0.Final Jul 21, 2017

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Jul 21, 2017

Member

I have changed the milestone of this one, as it was fixed for 1.2.0.Beta2 actually. I have updated the migration notes as well.

Member

filiphr commented Jul 21, 2017

I have changed the milestone of this one, as it was fixed for 1.2.0.Beta2 actually. I have updated the migration notes as well.

@filiphr filiphr changed the title from @InheritReverseConfiguration cannot directly inherit from prototype methods to @InheritInverseConfiguration cannot directly inherit from prototype methods Jul 21, 2017

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