-
Notifications
You must be signed in to change notification settings - Fork 211
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
Add JavaTimeModule #1065
Add JavaTimeModule #1065
Conversation
853f868
to
0c1886f
Compare
Ping @mcruzdev were you still working on this? |
Hi @cicoyle to be sincerous no, but I would like to continue, do you think to get this one? |
I need to add tests and probably to upgrade the dependency version |
c633534
to
d087fea
Compare
Hi @cicoyle, sorry for delay... I think this one solve the issue. I tested calling the |
@artursouza can you check this? I am not sure to have the full context to evaluate these changes.. but looking at the code it looks ok to me. Do we want to have that new dependency in the SDK? I am ok with it. |
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.
@mcruzdev thank you for your contribution, it looks really good!
sdk/src/test/java/io/dapr/serializer/DefaultObjectSerializerTest.java
Outdated
Show resolved
Hide resolved
@mcruzdev as I was thinking about your PR, it occurred to me that there could be a lot of other instances where we would like to register a custom Jackson module. My suggestion is to see how can we expand the
CC: @cicoyle @artursouza |
Good suggestions @artur-ciocanu TY, I will update the PR. |
Hi @artur-ciocanu, could you take a new review? |
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.
@mcruzdev thanks a lot for the updates, I have provided some additional comments, also I think it would make sense to provide some examples on how anyone could customize the serializers.
I think it would make sense to have a separate unit test for CustomizableObjectSerializer
where we could show how one could customize the serializer and how ZonedDateTime would be serialized using Jackson JavaTimeModule
sdk/src/main/java/io/dapr/serializer/CustomizableDaprObjectSerializer.java
Outdated
Show resolved
Hide resolved
ab07e07
to
9f98ac2
Compare
Signed-off-by: Matheus Cruz <[email protected]>
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.
@mcruzdev thanks for contribution, love it. I have left a few tiny comments.
sdk/src/main/java/io/dapr/serializer/ObjectSerializerFactory.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Matheus Cruz <[email protected]>
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.
Awesome job! 👍
@cicoyle I think this PR looks great and it is a good example on how anyone can extend the Could you please review and approve. Thank you! |
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.
Approving for CI
@cicoyle could you please approve this PR. Thank you. |
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.
LGTM ! thanks @mcruzdev !
retriggering CI but this is ready to be merged @cicoyle |
@cicoyle could you please approve and merge this PR. Thank you. |
Thanks @mcruzdev 🎉 |
Description
Please explain the changes you've made
Issue reference
#995
We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.
Please reference the issue this PR will close: #[issue number]
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: