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

Replace ZonedDateTime with valid String representation #80

Closed
wants to merge 5 commits into from
Closed

Replace ZonedDateTime with valid String representation #80

wants to merge 5 commits into from

Conversation

dturanski
Copy link

Resolves #29

@dturanski
Copy link
Author

Hold up on this. I'm working on some tweaks.

@dturanski
Copy link
Author

I pushed another commit to make it better

@fabiojose
Copy link
Contributor

@dturanski

Please, sign off the commits.

Replace ZonedDateTime with valid String representation
a0fea8f 
Signed-off-by: David Turanski <dturanski@pivotal.io>
fabiojose
fabiojose reviewed Dec 6, 2019
View reviewed changes
Copy link
Contributor

@fabiojose fabiojose left a comment

Choose a reason for hiding this comment

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

@dturanski

We need tests for this modification.

Please, provide that.

api/src/main/java/io/cloudevents/v03/AttributesImpl.java Show resolved Hide resolved
api/src/test/java/io/cloudevents/v1/CloudEventJacksonTest.java
@@ -345,5 +341,4 @@ public void should_marshal_data_byte_array_as_data_base64() {
assertTrue(encoded.contains("\"data_base64\""));
assertTrue(encoded.contains("\"" + expected +"\""));
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Please, avoid pushing files just with empty line remove.

api/src/test/java/io/cloudevents/v02/CloudEventJacksonTest.java Outdated
@@ -20,9 +20,15 @@
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;

import com.google.gson.Gson;
Copy link
Contributor

Choose a reason for hiding this comment

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

I did not able to see where you are using this import.

@dturanski
Copy link
Author

The current test suite verifies that we didn't break anything. Note that the gson import is left over from an attempt to test improved interoperability. Interoperability motivated this PR, but as I commented in the issue, this fix doesn't help with that since there are additional Jackson dependencies. My use case involved previous versions of cloud events and and the projectriff/java-function-invoker which was using gson internally to deserialize json payloads. As it turns out, the JFI uses Jackson now. GSon still doesn't work for other reasons. All attributes are null for example. So maybe the additional test is that I should be able to use ObjectMapper out of the box with no special config vs. requiring JSon. I'll add that.

Change AttributeImpl.time to String
62a9fd9 
Signed-off-by: David Turanski <dturanski@pivotal.io>
David Turanski added 3 commits December 7, 2019 11:19
Remove gson dependency
8e469c6 
Signed-off-by: David Turanski <dturanski@pivotal.io>
Fix version mixup and add Jackson Annotations
14cb37c 
Signed-off-by: David Turanski <dturanski@pivotal.io>
Replace incorrect (v1) types
d09622a 
Signed-off-by: David Turanski <dturanski@pivotal.io>
@fabiojose fabiojose requested a review from matzew December 18, 2019 13:03
@slinkydeveloper
Copy link
Member

Thank you for your contribution, we are reworking the sdk and this PR now is invalid. Look here: #29 (comment)

@slinkydeveloper slinkydeveloper added the invalid This doesn't seem right label Apr 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabiojose fabiojose fabiojose left review comments

@matzew matzew Awaiting requested review from matzew

Assignees
No one assigned
Labels
invalid This doesn't seem right
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ZonedDateTime field limits interoperability
3 participants
@dturanski @fabiojose @slinkydeveloper