search

vitruv-tools / Vitruv

View-based Development and Model Consistency Framework
http://vitruv.tools
Eclipse Public License 1.0
14 stars 19 forks source link

Simplify NotificationToEChangeConverter #361

Closed jGleitz closed 3 years ago

jGleitz commented 3 years ago

:warning: depends https://github.com/kit-sdq/SDQ-Commons/pull/27 :warning:

While reading through NotificationToEChangeConverter, I found the logic quite involved and had troubles to follow it. I have refactored it to a form that I find easier to follow. The code jumps less and the main structure can now directly be grasped from the convert method. I have also extracted often-used patterns into helper methods.

For my mind, these changes are improvements. I am happy to discuss all of them!

jGleitz commented 3 years ago

@HeikoKlare can you please publish a release of the SDQ Commons to make this PR build?

HeikoKlare commented 3 years ago

I have added a minor fix, improving the updatesite version for SDQ-Commons, which was only improved for repository used by Maven before. Without that change, dependent projects would not find the recently added methods, which are used in the changes implemented by this PR.

jGleitz commented 3 years ago

The creation of recursive changes (for creations/deletions) is, as far as I know, not used and tested anywhere.

Recursive changes for creation are partially tested by the change tests you wrote. I don’t think the tests cover all cases, but they at least caught my first implementation, which did a simple breadth-first search instead of paying attention to the order.

HeikoKlare commented 3 years ago

It seems like the modified change converter delivers better results, because the UML models created in the MediaStoreTest now contain InterfaceRealizations for provided roles in PCM, which they did not contain before. I will further investigate this, but just wanted to let you know that probably there is no bug in this PR, but probably an unrecognized bug was fixed.

jGleitz commented 3 years ago

but probably an unrecognized bug was fixed

That was not my intention, but hey, that is good! :smiley:

HeikoKlare commented 3 years ago

I will merge this PR. As mentioned before, the test failures in the CBS commonalities tests were caused by the modified change converter producing somehow "better" results (although I did not investigate why and where they actually are "better"). Fortunately, a fault in a matcher revealed the errors, because the intended matcher implementation would have simply swallowed the changed results ;-)

jGleitz commented 3 years ago

Fortunately, a fault in a matcher revealed the errors, because the intended matcher implementation would have simply swallowed the changed results ;-)

Sometimes, errors cancel out :laughing: