Opened 5 years ago

Closed 4 years ago

#1923 closed enhancement (done)

CheckTypeCompatibility does not check constraints on generic type parameters

Reported by: mkommend Owned by: gkronber
Priority: medium Milestone: HeuristicLab 3.3.8
Component: PluginInfrastructure Version: 3.3.8
Keywords: Cc:

Description

The CheckTypeCompatibility in all ApplicationManagers could be further optimized by checking the constraints of generic type parameters in addition to the creation of concrete types and assigning the generic types.

Change History (16)

comment:1 Changed 5 years ago by mkommend

  • Owner changed from gkronber to mkommend
  • Status changed from new to accepted

comment:2 Changed 5 years ago by mkommend

Accidentally commited changes in LightweightApplicationManager and SandboxApplicationManager with r8531, which adds a check for the new and class constraint on generic types.

comment:3 Changed 5 years ago by mkommend

  • Owner changed from mkommend to gkronber
  • Status changed from accepted to reviewing

The change in r8531 is only a minor change in the PluginInfrastructure as no new method got added or method signature changed. Only the behavior of the CheckTypeCompatibility was enhanced by additional constraint checks. Furthermore, I have tested the changeset thoroughly, as I worked locally with this modified version for over two weeks.

comment:4 Changed 5 years ago by mkommend

  • Owner changed from gkronber to mkommend
  • Status changed from reviewing to assigned

comment:5 Changed 5 years ago by mkommend

  • Status changed from assigned to accepted

In r8531 an error was introduced regarding the discovery of generic types and some unit tests failed during the build.

comment:6 Changed 5 years ago by mkommend

r8536: Reverse merged r8531 (changes in application managers) as this introduced errors in the type discovery.

comment:7 Changed 5 years ago by mkommend

  • Owner changed from mkommend to gkronber
  • Status changed from accepted to assigned

The unit test should pass now, since the changes got reverted. However, I am not completely sure if the TypeCompatibilityCheck works correctly for generic types. We should discuss this issue and that's why I leave this ticket open.

comment:8 Changed 5 years ago by mkommend

It was decided to specify the behavior of the type discovery by unit tests and to reimplement the type compatibility check.

comment:9 Changed 5 years ago by mkommend

  • Owner changed from gkronber to mkommend
  • Status changed from assigned to accepted

comment:10 Changed 5 years ago by mkommend

  • Status changed from accepted to reviewing

r8571:

  • Rewrote type checks for type discovery of plugin infrastructure
  • Extracted common functionality of ApplicationManagers in another class.
  • Added unit tests for the newly implemented methods*
  • Updated test lists to include the new unit tests

comment:11 Changed 5 years ago by mkommend

  • Owner changed from mkommend to gkronber

comment:12 Changed 5 years ago by mkommend

  • Owner changed from gkronber to mkommend
  • Status changed from reviewing to assigned

It was decided that generic types should only be returned if the generic type parameter is exactly the same.

comment:13 Changed 5 years ago by mkommend

r8577: Changed type discovery to return only generic types with exactly the same type parameter and adapted the unit test accordingly.

comment:14 Changed 5 years ago by mkommend

  • Owner changed from mkommend to gkronber
  • Status changed from assigned to reviewing

Please feel free to extend the test coverage by adding more test cases during the review.

comment:15 Changed 4 years ago by gkronber

  • Status changed from reviewing to readytorelease

Reviewed r8571 and r8577. The implementation is very clear and readable and tested trough unit tests. I'm happy with the change even though type discovery is still very slow (see #1972)

comment:16 Changed 4 years ago by swagner

  • Resolution set to done
  • Status changed from readytorelease to closed
  • Version changed from 3.3.7 to 3.3.8
Note: See TracTickets for help on using tickets.