-
Notifications
You must be signed in to change notification settings - Fork 7
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
Migrate to UUIDs #47
Migrate to UUIDs #47
Conversation
c350785
to
563356b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
bundles/tools.vitruv.change.atomic/src/tools/vitruv/change/atomic/EChangeUuidManager.xtend
Outdated
Show resolved
Hide resolved
Missing tests for UUID resolver were added and code suggestions implemented.
As such, this PR (and its counterparts in the other repositories) is ready to merge. @vitruv-tools/maintainers |
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.
Reviewed the changes of the implementation code (no tests)
bundles/tools.vitruv.change.atomic/src/tools/vitruv/change/atomic/uuid/UuidResolverImpl.java
Show resolved
Hide resolved
bundles/tools.vitruv.change.atomic/src/tools/vitruv/change/atomic/uuid/UuidResolverImpl.java
Outdated
Show resolved
Hide resolved
....vitruv.change.atomic/src/tools/vitruv/change/atomic/resolve/AtomicEChangeUuidResolver.xtend
Show resolved
Hide resolved
....vitruv.change.atomic/src/tools/vitruv/change/atomic/resolve/AtomicEChangeUuidResolver.xtend
Show resolved
Hide resolved
....vitruv.change.atomic/src/tools/vitruv/change/atomic/resolve/AtomicEChangeUuidResolver.xtend
Show resolved
Hide resolved
....change.atomic/src/tools/vitruv/change/atomic/resolve/EChangeUuidResolverAndApplicator.xtend
Outdated
Show resolved
Hide resolved
bundles/tools.vitruv.testutils/src/tools/vitruv/testutils/views/ChangePublishingTestView.xtend
Outdated
Show resolved
Hide resolved
bundles/tools.vitruv.testutils/src/tools/vitruv/testutils/views/ChangePublishingTestView.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.
Reviewed the tests.
...uv.change.atomic.tests/src/tools/vitruv/change/atomic/compound/RemoveAndDeleteRootTest.xtend
Show resolved
Hide resolved
...s.vitruv.change.atomic.tests/src/tools/vitruv/change/atomic/root/RemoveRootEObjectTest.xtend
Outdated
Show resolved
Hide resolved
.../tools.vitruv.change.atomic.tests/src/tools/vitruv/change/atomic/uuid/UuidResolvingTest.java
Outdated
Show resolved
Hide resolved
.../tools.vitruv.change.atomic.tests/src/tools/vitruv/change/atomic/uuid/UuidResolvingTest.java
Outdated
Show resolved
Hide resolved
Suggestions are implemented, force pushing seems to cause some problems.
replace IdResolver with UuidResolver
added UUID resolver (de)serialization
support registration of read-only EObjects in UUID resolver
…urning a resource
…ed by Java Editor - PCM legacy case study
This PR replaces hierarchical IDs (HIDs) with UUIDs for the internal consistency preservation. A UUID uniquely identifies an element but instead of a HID cannot be generated on the fly and is permanent. As such, any element must be registered to the
UUIDResolver
explicitly. Since elements are identified based on their identity and not their hierarchical position anymore, when copying aResource
, theUUIDResolver
cannot be reused but a new one has to be created, matching the elements of the original resolver. As this case happens frequently,UUIDResolver::resolveResources
is introduced.This PR is introduced to enable the
VirtualModel
to identify all its elements and correspondences based on permanent identifiers, such that versioning, variants, and distributed functionalities are easier to support.Since HIDs are still needed (in views), currently there are two similar class structures for HIDs and UUIDs. As next step, we should come up with one single interface to hide both functionalities. From my experience working on this PR, I suggest to aim for hiding almost all the currently exposed
IDResolver
logic and instead only provide functionality to assign IDs to a change or to resolve a change with the appropriate IDs assigned. As such, we should investigate whether changes can be made generic over the type of content (EObject
, HID, UUID) they store (see #58).Changes in detail
UUIDResolver
and matching classes for assigning UUIDs to changes and applying and resolving changes containing UUIDsChangeRecorder
to reduce the occasions where anIDResolver
has to be exposedChangeRecordingModelRepository
withUUIDResolver
ChangePublishingTestView
to load the needed UUIDs into its own resolver when loading a resourceFollow-up tasks
EChange
generic over its content type #59UuidResolver
#60