New Adapter: RoundhouseAds with v3 compatibility #4037
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Code coverage summaryNote:
roundhouseadsRefer here for heat map coverage report |
|
|
||
| maintainer: | ||
| email: "[email protected]" | ||
| #gvlVendorID: 42 #need assigned ID |
There was a problem hiding this comment.
Please resolve this todo item.
| userSync: | ||
| # Roundhouseads supports user syncing, but requires configuration by the host. Contact a technical account manager | ||
| # or this bidder directly at the email address in this file to ask about enabling user sync. | ||
| endpointCompression: gzip |
There was a problem hiding this comment.
endpointCompression is not a member of userSync Please move this out of the userSync section.
| "properties": { | ||
| "publisherId": { | ||
| "type": "string", | ||
| "description": "An ID which identifies the publisher" |
There was a problem hiding this comment.
It's common to specify a minimum length of 1 to filter out empty strings.
| @@ -5,6 +5,7 @@ go 1.21 | |||
| retract v3.0.0 // Forgot to update major version in import path and module name | |||
|
|
|||
| require ( | |||
| github.com/51Degrees/device-detection-go/v4 v4.4.35 | |||
There was a problem hiding this comment.
Curious what caused these changes. I don't expect go.mod / go.sum changes in an adapter PR.
| `{}`, | ||
| `{"publisherId": 123456}`, | ||
| `{"publisherId": 0}`, | ||
| } |
There was a problem hiding this comment.
Please add a {"publisherId": null} case as well.
| modifiedImps = append(modifiedImps, imp) | ||
| } | ||
|
|
||
| request.Imp = modifiedImps |
There was a problem hiding this comment.
Nitpick: You're making a new imp slice and then setting that back to the request. You can edit the imps directly instead to avoid the extra allocations.
| if err != nil { | ||
| errs = append(errs, err) | ||
| return nil, errs | ||
| } |
There was a problem hiding this comment.
Please do not preallocate an errors slice. It's only used on line 86, which can be converted to:
return nil, []error{err}
| Body: reqJSON, | ||
| Headers: headers, | ||
| ImpIDs: openrtb_ext.GetImpIDs(request.Imp), | ||
| }}, errs |
There was a problem hiding this comment.
It's not possible in this code for there to be an error at this point. Please hardcode a nil return.
| } | ||
|
|
||
| if roundhouseadsExt.PublisherId != "" { | ||
| return roundhouseadsExt.PublisherId, nil |
There was a problem hiding this comment.
Please clearly indicate in your accompanying prebid.github.io PR that only the first impression publisher is used and the rest are ignored.
| if !isValidEndpoint || err != nil { | ||
| return nil, errors.New("ExtraAdapterInfo must be a simple string provided by Roundhouseads") | ||
| } | ||
| } |
There was a problem hiding this comment.
Can your adapter run successfully if there is no extra adapter info provided?
New Adapter: RoundhouseAds with v3 compatibility