Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve Codec handling for multi module use #231

Merged
merged 6 commits into from
Jul 28, 2022
Merged

Improve Codec handling for multi module use #231

merged 6 commits into from
Jul 28, 2022

Conversation

eneufeld
Copy link
Contributor

@eneufeld eneufeld commented Jul 8, 2022

Currently Codecs are only registered using the format they support.
This is not enough if different codecs are needed for the same
format but different models.
The new API uses a CodecProvider that can be used with a bazaar pattern
so that the best fitting CodecProvider can be retrieved.

Fix #230

@eneufeld eneufeld requested review from cdamus and ndoschek July 8, 2022 11:48
cdamus
cdamus requested changes Jul 8, 2022
View reviewed changes
Copy link
Contributor

@cdamus cdamus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @eneufeld for the contribution! This is a big improvement over the current design.

...ud.modelserver.emf/src/org/eclipse/emfcloud/modelserver/emf/common/codecs/CodecProvider.java Show resolved Hide resolved
...ud.modelserver.emf/src/org/eclipse/emfcloud/modelserver/emf/common/codecs/CodecProvider.java Show resolved Hide resolved
...server.emf/src/org/eclipse/emfcloud/modelserver/emf/common/codecs/DefaultCodecsProvider.java Outdated Show resolved Hide resolved
...ud.modelserver.emf/src/org/eclipse/emfcloud/modelserver/emf/di/DefaultModelServerModule.java Outdated Show resolved Hide resolved
...fcloud.modelserver.emf/src/org/eclipse/emfcloud/modelserver/emf/patch/EMFCommandHandler.java Show resolved Hide resolved
....emfcloud.modelserver.emf/src/org/eclipse/emfcloud/modelserver/emf/util/JsonPatchHelper.java Outdated Show resolved Hide resolved
....emfcloud.modelserver.emf/src/org/eclipse/emfcloud/modelserver/emf/util/JsonPatchHelper.java Outdated Show resolved Hide resolved
@cdamus
Copy link
Contributor

cdamus commented Jul 8, 2022

Oops, sorry, I didn't notice that this PR was just a draft and now I cannot dismiss my review because it's a draft.

cdamus
cdamus reviewed Jul 11, 2022
View reviewed changes
Copy link
Contributor

@cdamus cdamus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates! Sorry, I have new requests for changes that I should have found the first time around.

...ud.modelserver.emf/src/org/eclipse/emfcloud/modelserver/emf/common/codecs/CodecProvider.java Show resolved Hide resolved
...ud.modelserver.emf/src/org/eclipse/emfcloud/modelserver/emf/common/codecs/CodecProvider.java Outdated Show resolved Hide resolved
....modelserver.emf/src/org/eclipse/emfcloud/modelserver/emf/common/codecs/DICodecsManager.java Outdated Show resolved Hide resolved
....modelserver.emf/src/org/eclipse/emfcloud/modelserver/emf/common/codecs/DICodecsManager.java Outdated Show resolved Hide resolved
@eneufeld
Copy link
Contributor Author

good catches, I will add those, thank you!

cdamus
cdamus approved these changes Jul 11, 2022
View reviewed changes
Copy link
Contributor

@cdamus cdamus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! I don't know what the problem is with the build. Works fine for me, including the tests.

@cdamus cdamus self-requested a review July 11, 2022 21:17
@ndoschek
Copy link
Contributor

Thanks Eugen! I didn't have time to look into it yet, but regarding the build, the EMF tests are failing that are only included in the m2 build (due to known issues in the p2 environment, see also #167).

cdamus
cdamus requested changes Jul 12, 2022
View reviewed changes
Copy link
Contributor

@cdamus cdamus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying that, @ndoschek . Indeed, the test failures seem to be finding XMI encodings where JSON encodings were expected. The same tests fail when running the maven build with m2 profile and from the Eclipse launch configuration.

@eneufeld eneufeld requested a review from cdamus July 14, 2022 09:56
@eneufeld
Copy link
Contributor Author

Good thing we have the tests running ;-) .
Thank you for seeing this.
Updated the test mock setup to fix the test.

cdamus
cdamus approved these changes Jul 14, 2022
View reviewed changes
Copy link
Contributor

@cdamus cdamus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to work. Thanks!

ndoschek
ndoschek requested changes Jul 18, 2022
View reviewed changes
Copy link
Contributor

@ndoschek ndoschek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot Eugen! Overall it looks good to me, I added some minor nitpicks inline.
Would be great if you could have a look, thanks!

...ud.modelserver.emf/src/org/eclipse/emfcloud/modelserver/emf/common/codecs/CodecsManager.java Show resolved Hide resolved
...ud.modelserver.emf/src/org/eclipse/emfcloud/modelserver/emf/common/codecs/CodecProvider.java Outdated Show resolved Hide resolved
...ud.modelserver.emf/src/org/eclipse/emfcloud/modelserver/emf/common/codecs/CodecProvider.java Outdated Show resolved Hide resolved
....modelserver.emf/src/org/eclipse/emfcloud/modelserver/emf/common/codecs/DICodecsManager.java Outdated Show resolved Hide resolved
....modelserver.emf/src/org/eclipse/emfcloud/modelserver/emf/common/codecs/DICodecsManager.java Show resolved Hide resolved
....modelserver.emf/src/org/eclipse/emfcloud/modelserver/emf/common/codecs/DICodecsManager.java Outdated Show resolved Hide resolved
...server.emf/src/org/eclipse/emfcloud/modelserver/emf/common/codecs/DefaultCodecsProvider.java Outdated Show resolved Hide resolved
....emfcloud.modelserver.emf/src/org/eclipse/emfcloud/modelserver/emf/util/JsonPatchHelper.java Outdated Show resolved Hide resolved
....emfcloud.modelserver.emf/src/org/eclipse/emfcloud/modelserver/emf/util/JsonPatchHelper.java Outdated Show resolved Hide resolved
...fcloud.modelserver.emf/src/org/eclipse/emfcloud/modelserver/emf/patch/EMFCommandHandler.java Show resolved Hide resolved
eneufeld added 5 commits July 19, 2022 08:44
@eneufeld
Improve Codec handling for multi module use
3f1e2b2 
Currently Codecs are only registered using the format they support.
This is not enough if different codecs are needed for the same
format but different models.
The new API uses a CodecProvider that can be used with a bazaar pattern
so that the best fitting CodecProvider can be retrieved.

Fix eclipse-emfcloud#230
@eneufeld
Address review comments
7400b4d 
- add static methods to CodecProvider
- change DefaultCodecsProvider to have a map instead of a Set
- add JavaDoc
@eneufeld
More review fixes
a7fdfff
@eneufeld
Fix tests
cb4ed32
@eneufeld
Fix more review comments
f353783
ndoschek
ndoschek reviewed Jul 19, 2022
View reviewed changes
Copy link
Contributor

@ndoschek ndoschek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update Eugen! Looks good to me, two final minors from my side. Thanks!

...ud.modelserver.emf/src/org/eclipse/emfcloud/modelserver/emf/common/codecs/CodecsManager.java Outdated Show resolved Hide resolved
...d.modelserver.emf/src/org/eclipse/emfcloud/modelserver/emf/common/watchers/ModelWatcher.java Show resolved Hide resolved
...ud.modelserver.emf/src/org/eclipse/emfcloud/modelserver/emf/common/codecs/CodecProvider.java Outdated Show resolved Hide resolved
ndoschek
ndoschek approved these changes Jul 19, 2022
View reviewed changes
Copy link
Contributor

@ndoschek ndoschek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot Eugen! 👍

@cdamus cdamus marked this pull request as ready for review July 28, 2022 13:07
@cdamus cdamus merged commit 3093d55 into eclipse-emfcloud:master Jul 28, 2022
@cdamus cdamus mentioned this pull request Aug 3, 2022
Closed
@cdamus cdamus mentioned this pull request Aug 22, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdamus cdamus cdamus approved these changes

@ndoschek ndoschek ndoschek approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for multiple modules
3 participants
@eneufeld @cdamus @ndoschek