-
Notifications
You must be signed in to change notification settings - Fork 61
Conversation
openrtb2/audio.go
Outdated
Protocols []Protocol `json:"protocols,omitempty"` | ||
// Array of supported audio protocols. Refer to List: Creative | ||
// Subtypes - Audio/Video in AdCOM 1.0. | ||
Protocols []int8 `json:"protocols,omitempty"` |
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.
All enumeration lists (formerly section 5) have been removed from OpenRTB 2.6 in favor of the definitions in AdCOM 1.0. I disassociated the enums for now, but do you think it's better to directly reference the /adcom1 package here? .. or copy the AdCOM enums to /openrtb2?
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 commented above, I think it will be better to import adcom1 in openrtb2 since it relies so heavily on adcom now.
I'll get back to it once done with other bits.
I expected 2.6 to result in fewer changes, that's quite a big PR. Thanks a lot! Also preparing next major release (boring service work) - much appreciated as well. I left a couple of 👍 comments, and only quickly eyeballed the diff - so far seems good. I'll do a proper/detailed review (comparing to 2.6 spec if anything's missed + what to do with Audio.Protocols type) over this weekend, so expecting a release (v16) somewhere around next Mon-Wed. |
Agreed. There were more changes than I expected.
Happy to help. :)
Cool. Thank you. |
openrtb2/audio.go
Outdated
Delivery []ContentDeliveryMethod `json:"delivery,omitempty"` | ||
// none specified, assume all are supported. Refer to List: Delivery | ||
// Methods in AdCOM 1.0. | ||
Delivery []int8 `json:"delivery,omitempty"` |
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.
TODO: maybe import adcom1 here
openrtb2 top-to-bottom review: i left off at device. I was working through the enums the other day instead of the openrtb2 objects. I switched to match. i have a couple of comments/questions on how to handle some spec differences. |
I completed the top-to-bottom review up until the UID object. fixed some errata. should all be good now. |
Mostly for enum shift (206 and higher IDs) introduced in: InteractiveAdvertisingBureau/openrtb@5a6aac5 > 3.0 September 2021 Clarified loss reason: Loss Reason Code 206 was renamed from Bundle to Store ID to keep the intent of this loss reason consistent across OpenRTB 2.x and 3.x.
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 consider this as good for now.
I'll merge it now, then will release a v16-alpha.
Maybe something will pop up when bumping this lib where used (maybe https://github.com/prebid/prebid-server etc).
Note: addresses prebid/prebid-server#2139 |
Fantastic. Thank you for working with me on this PR.
That's my project. :) I don't foresee any issues, but there may be surprises when we start going through the code. |
Implements #47. Includes all changes in OpenRTB 2.6 and parity changes in AdCOM 1.0.