-
Notifications
You must be signed in to change notification settings - Fork 744
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
Smaato: Send imp.ext object entirely #3995
base: master
Are you sure you want to change the base?
Conversation
Code coverage summaryNote:
smaatoRefer here for heat map coverage report
|
4d58f1f
to
ae02b61
Compare
Code coverage summaryNote:
smaatoRefer here for heat map coverage report
|
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.
Looks good to me
Hi @Enigo, can you please merge with master (no rebase) and resolve conflicts? Thanks! |
Code coverage summaryNote:
smaatoRefer here for heat map coverage report
|
@bsardo |
@bsardo |
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.
Looking good, just one nitpick.
adapters/smaato/smaato.go
Outdated
@@ -513,33 +514,33 @@ func setImpForAdBreak(imps []openrtb2.Imp) error { | |||
imps[i].Video = &videoCopy | |||
} | |||
|
|||
imps[0].Ext = impExt | |||
imps[0].Ext = ext |
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.
Nitpick: I think you can get rid of the ext
variable and instead just assign firstImp.Ext
to imps[0].Ext
.
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.
fixed
err := jsonparser.ObjectEach(updatedExt, func(key []byte, value []byte, dataType jsonparser.ValueType, offset int) error { | ||
isEmpty = false | ||
return nil | ||
}) |
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.
👍 this is an interesting approach. I imagine this is much faster than unmarshaling to a map and checking the length.
Code coverage summaryNote:
smaatoRefer here for heat map coverage report
|
This PR refactors the way how our adapter sends the
imp.ext
object to the ad server - now the entire object is sent and not onlyskadn