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.

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

Reworking datacodec package #428

Merged

Conversation

slinkydeveloper
Copy link
Member

With this PR datacodec package doesn't handle the base64 encoding, this is left to the event marshaller/unmarshaller which injects into the Data field directly the decoded byte array.

Also improves documentation of SetData and Data explaining the various behaviours.

@slinkydeveloper slinkydeveloper added this to the SDK v2 milestone Mar 30, 2020
@@ -66,7 +66,7 @@ func TestMarshal(t *testing.T) {
"exurl": source,
"extime": &now,
},
want: toBytes(map[string]interface{}{
Copy link
Member

Choose a reason for hiding this comment

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

In this file we need to add a test for each of the other encodings the sdk supports

@n3wscott
Copy link
Member

LGTM

We need to do a pass at xml testing, I am seeing a regression in the sdk (unrelated to this PR)

…d from base64

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Improved tests for base64 encoding case

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
@slinkydeveloper slinkydeveloper force-pushed the reworking_codec_package branch from dbac0fe to dd6bb7b Compare March 30, 2020 16:51
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
@n3wscott
Copy link
Member

nice.

LGTM, thanks for cleaning up.

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
@slinkydeveloper slinkydeveloper merged commit 6dc020a into cloudevents:master Mar 30, 2020
@slinkydeveloper slinkydeveloper deleted the reworking_codec_package branch March 30, 2020 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants