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

Refactor Resource Saving #309

Closed jGleitz closed 3 years ago

jGleitz commented 3 years ago

This PR implements the changes @HeikoKlare and I discussed in #308. Since we realised that changes to multiple models are currently forbidden, this PR mainly reduces the API surface of VitruvApplicationTest and implements one universal recordAndPropagate function.

I think this is an improvement because it reduces the API to only a few helpers, which are easy to understand. The major downside I see is that recordAndPropagate is quite a long name for an often used function.

HeikoKlare commented 3 years ago

Just as a quick remark: I am not completely convinced that merging recording and propagation into one method is the optimal solution. With my previous remark I only intended to have a kind of saveAndPropagate method, just like the existing saveAndSynchronizeChanges method, which does not expect a resource as a parameter but instead retrieves it from the change recorder. Merging this also with the recording removes the ability to start multiple recording transactions for different elements (of the same resource) and then, at some point, issue resource saving and change propagation. You can still do it with the proposed changes, but you then have to save the (correct) resource on your own, although it could be derived from the recordings.

The use case implemented by recordAndPropagate may be the most relevant and thus should be provided at least as a convenience method, but I would still expect the general sequence to be as follows: one or more record calls issue the recording for one or more elements, for which the method dynamically checks (e.g. managed by the change recorder) that only a single resource is affected; then a saveAndPropagate call saves the unique resource modified by the previous record calls and issues change propagation by means of the existing propagateChanges method. The simple and most often used process only involving one record call is then wrapped by the recordAndPropagate method.

jGleitz commented 3 years ago

That makes sense.

jGleitz commented 3 years ago

I changed the logic so that we have saveAndPropagate. I also renamed recordAndPropagate to only propagate since I think that that is clear enough.

HeikoKlare commented 3 years ago

The PR is in general fine for me and ready to merge. I have commited the assignment of the logic for determining the resource affected by a change to the change classes (instead of the VitruvApplicationTest). Consider this as a proposal for improvement. Do you agree to that modification, @jGleitz?

jGleitz commented 3 years ago

Moving this logic to the changes makes a lot of sense :+1: