-
Notifications
You must be signed in to change notification settings - Fork 19
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
Separate change propagation from VirtualModel #501
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made pretty picky comments as these code parts will probably not be touched for a while.
In general I like the proposed changes, I am just questioning myself whether the approach to work directly on the nsURIs is the best. It makes the accessor names pretty long and we lose some type safety. The alternative would be to introduce some lightweight interface like ChangeDomain
or similar that encapsulates the nsURIs. However, having an interface to only encapsulate one property can be considered as unnecessary overhead.
What were your decisive arguments to throw away the domain concept in the change context?
...mmonalities/src/tools/vitruv/dsls/commonalities/generator/reactions/ReactionsGenerator.xtend
Outdated
Show resolved
Hide resolved
bundles/dsls/tools.vitruv.dsls.reactions/model/TopLevelElements.ecore
Outdated
Show resolved
Hide resolved
...work.change/src/tools/vitruv/framework/change/description/impl/TransactionalChangeImpl.xtend
Outdated
Show resolved
Hide resolved
....vitruv.framework.change/src/tools/vitruv/framework/change/description/VitruviusChange.xtend
Outdated
Show resolved
Hide resolved
....framework.domains/src/tools/vitruv/framework/domains/repository/VitruvDomainRepository.java
Show resolved
Hide resolved
...itruv.framework.propagation/src/tools/vitruv/framework/propagation/ChangeInPropagation.xtend
Show resolved
Hide resolved
...work.propagation/src/tools/vitruv/framework/propagation/ChangePropagationSpecification.xtend
Outdated
Show resolved
Hide resolved
...utils/tools.vitruv.testutils/src/tools/vitruv/testutils/ViewBasedVitruvApplicationTest.xtend
Show resolved
Hide resolved
bundles/testutils/tools.vitruv.testutils/src/tools/vitruv/testutils/VitruvApplicationTest.xtend
Show resolved
Hide resolved
Thanks for the review. The reason why we want to get rid of the domain is that they are a concept that is only used in the |
...work.propagation/src/tools/vitruv/framework/propagation/ChangePropagationSpecification.xtend
Outdated
Show resolved
Hide resolved
...work.propagation/src/tools/vitruv/framework/propagation/ChangePropagationSpecification.xtend
Outdated
Show resolved
Hide resolved
I totally agree with your arguments. The benefit I see with an interface - besides shorter attribute names - was the possibility to have a centralized documentation (like how to retrieve nsURIs or what they represent). If using a data class, we could even implement some validation to the URIs. However, these are not the strongest arguments to motivate a refactoring. Additionally, as I'm coming from the Swift language domain where interfaces are far more lightweight, maybe I am a bit biased on the overhead of such an additional interface. |
bundles/dsls/tools.vitruv.dsls.reactions/model/TopLevelElements.genmodel
Outdated
Show resolved
Hide resolved
Regarding the namespace URIs: I agree that encapsulating the URIs makes sense. I've proposed a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the encapsulating is as I would have expected it.
Regarding the naming: I also think that metamodel is suboptimal, as the class itself does not model anything or contain model elements.
I was thinking about what we really want to express with the class. The nsURIs are the location of the metamodel of the domain. The class itself has no actual functionality but serves as a wrapper. Regarding overloading of the "domain" word, are there other concepts than the VitruvDomain
?
I was thinking of DomainDescriptor
/ DomainDescription
or similar? And if we want to be precise we could name the uri attribute then metamodelNsUris
.
....vitruv.framework.change/src/tools/vitruv/framework/change/description/VitruviusChange.xtend
Outdated
Show resolved
Hide resolved
...rk/tools.vitruv.framework.propagation/src/tools/vitruv/framework/propagation/Metamodel.xtend
Outdated
Show resolved
Hide resolved
...rk/tools.vitruv.framework.propagation/src/tools/vitruv/framework/propagation/Metamodel.xtend
Outdated
Show resolved
Hide resolved
There are no other concepts except for the |
...ruv.framework.change/src/tools/vitruv/framework/change/description/TransactionalChange.xtend
Outdated
Show resolved
Hide resolved
....vitruv.framework.change/src/tools/vitruv/framework/change/description/CompositeChange.xtend
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
I only added some minor comments regarding documentation.
This PR separates the change propagation concepts of the framework from the
VirtualModel
. As a result,ChangePropagationSpecifications
do only depend on the specifications ofEChange
andVitruviusChange
as well as the user interaction definitions, but not onVitruvDomains
as aVirtualModel
-specific concept. In addition, theChangePropagator
that is currently integrated into theVirtualModel
concepts is made independent to be used without aVirtualModel
.This involves:
ChangePropagationSpecification
accepts namespace URIs of the metamodels rather thanVitruvDomains
ChangeInPropagation
that decoples the information about whether a change shall be propagated transitively from accessing theVitruvDomains
that are currently the only source for deciding on transitive propagation.ChangePropagator
:ChangeRecordingModelRepository
, which is implemented by theModelRepository
of theVirtualModel
, rather than onVitruvDomains
.VitruvDomains
to an explicit specification in aChangeInProgress
delivered by theChangeRecordingModelRepository
, which makes the decision for transitive propagation.With this PR, the
tools.vitruv.framework.propagation
bundle together with the change representation specifications can be used independent from the virtual model, domains and applications to implement change propagation. A subsequent PR (#502) will adapt the DSLs (Reactions, Mappings, Commonalities) to also depend on metamodel rather than domains, such that these DSLs can be used only depending on the change representation but not on the complete Vitruv framework.In a future PR, we may also add a default implementation for the
ChangeRecordingModelRepository
to have ready-to-use implementations for all change propagation artifacts to be used without aVirtualModel
, such that the DSLs can be easily used as standalone transformation languages.