New Adapter: VideoByte #2058
New Adapter: VideoByte #2058
Conversation
adapters/videobyte/videobyte.go
Outdated
| "github.com/mxmCherry/openrtb/v15/openrtb2" | ||
| ) | ||
|
|
||
| type VideoByteAdapter struct { |
There was a problem hiding this comment.
Please rename this to adapter. VideoByte in the name is redundant as the package name already contains videoByte
adapters/videobyte/videobyte.go
Outdated
| func (a *VideoByteAdapter) MakeRequests(request *openrtb2.BidRequest, reqInfo *adapters.ExtraRequestInfo) ([]*adapters.RequestData, []error) { | ||
| impressions := request.Imp | ||
| adapterRequests := make([]*adapters.RequestData, 0, len(impressions)) | ||
| errs := make([]error, 0, len(impressions)) |
There was a problem hiding this comment.
Please remove the capacity for the errs slice here. Specifying capacity will lead to Go creating an underlying array of that length in memory thus holding up that memory. This makes sense for adapterRequests because you expect the number of adapter requests to be equal to the number of impressions but for errors, we ideally expect it to be 0 and so we don't need to reserve memory for errors equal to number of impressions.
There was a problem hiding this comment.
Changed it so the slice is initially nil
|
|
||
| func getMediaTypeForImp(imp *openrtb2.Imp) openrtb_ext.BidType { | ||
| if imp != nil && imp.Banner != nil { | ||
| return openrtb_ext.BidTypeBanner |
There was a problem hiding this comment.
Is your bid server always guaranteed to respond with a banner bid for multi-format imps that may have both banner and video configured? If not, is there any indication in the bid returned about the media type? If so, I'd recommend using that to determine the media type of the bid
There was a problem hiding this comment.
Yes, it is guaranteed to respond with a banner bid for multi-format imps that have both video and banner.
adapters/videobyte/videobyte_test.go
Outdated
| ) | ||
|
|
||
| func TestJsonSamples(t *testing.T) { | ||
| adapterstest.RunJSONBidderTest(t, "videobytetest", &VideoByteAdapter{endpoint: "https://mock.videobyte.com"}) |
There was a problem hiding this comment.
Please consider using the Builder function to get an instance of the adapter to be passed into the RunJSONBidderTest function so that there's test coverage for that function too.
| errs := make([]error, 0, len(impressions)) | ||
|
|
||
| for _, impression := range impressions { | ||
| impExt, err := parseExt(&impression) |
There was a problem hiding this comment.
Please consider adding a JSON test with multiple imps and also a JSON test with a multi-format imp.
| params := url.Values{} | ||
| params.Add("source", "pbs") | ||
| params.Add("pid", impExt.PublisherId) | ||
| if impExt.PlacementId != "" { |
There was a problem hiding this comment.
Please consider including empty placement ID and network ID in one of your JSON test cases
There was a problem hiding this comment.
Added a test case.
| headers.Add("Accept", "application/json") | ||
|
|
||
| if request.Site != nil { | ||
| if request.Site.Domain != "" { |
There was a problem hiding this comment.
Please consider including empty Site.Domain and site.Ref in one of your JSON test cases
There was a problem hiding this comment.
Added a test case.
* 'master' of https://github.com/wwwyyy/prebid-server: (23 commits) Add GPID Reserved Bidder Name (prebid#2107) Marsmedia adapter - param zoneId can get string and int (prebid#2105) Algorix: add Server Region Support (prebid#2096) New Adapter: Bizzclick (prebid#2056) Collect Prometheus Go process metrics (prebid#2101) Added channel support (prebid#2037) Rubicon: update fpd fields resolving (prebid#2091) Beachfront - Currency conversion (prebid#2078) New Adapter: Groupm (Alias Of PubMatic) (prebid#2042) Adform adapter lacked gross/net parameter support (prebid#2084) add iframe sync for openx (prebid#2094) changes to the correct user sync url (prebid#2092) Smaato: Add instl to tests (prebid#2089) Correct MakeRequests() output assertion in adapter JSON tests (prebid#2087) Adview: adapter currency fix. (prebid#2060) New Adapter: Coinzilla (prebid#2074) Syncer Level ExternalURL Override (prebid#2072) 33Across: Enable Support for SRA requests (prebid#2079) Currency conversion on adapter JSON tests (prebid#2071) New Adapter: VideoByte (prebid#2058) ...
No description provided.