-
-
Notifications
You must be signed in to change notification settings - Fork 68
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
Convert ObjectMapper to interface #133
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.
Just a couple of signature changes to make, I think. I'll take another look when I'm less tired.
As this does have a breaking change, this will likely be a 4.x
change - so note that when this is ready, there may be a wait to pull until we have such a branch to accomodate it.
@@ -66,81 +53,42 @@ | |||
* @throws ObjectMappingException | |||
*/ | |||
@SuppressWarnings("unchecked") | |||
public static <T> ObjectMapper<T>.BoundInstance forObject(@NonNull T obj) throws ObjectMappingException { | |||
static <T> BoundInstance forObject(@NonNull T obj) throws ObjectMappingException { |
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 guess this could be BoundInstance<T>
?
|
||
public Class<T> getMappedType() { | ||
return this.clazz; | ||
Object getInstance(); |
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.
Should return T
.
I tried to minimalize all possible breaking changes, thats why for example BoundInstance remained as an inner class. If we do not care about how much will stuff break, we can start a discussion whenever it would be beneficial to move ObjectMapper, BoundInstance and all annotations to its own explicit gradle submodule/package. |
ObjectMapper converted from class to interface, including BoundInstance.
In a nutshell
ObjectMapper is now ObjectMapperImpl implementing ObjectMapper interface
No packages were changed or renamed.
Theres only one breaking change, which users might or might not encounter; Depends on usage
I tried to search places where sponge could be affected found only this:
https://github.com/SpongePowered/SpongeCommon/blob/stable-7/src/main/java/org/spongepowered/common/config/SpongeConfig.java#L117
Should become
ObjectMapper.BoundInstance<T>