-
Notifications
You must be signed in to change notification settings - Fork 748
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
Add Yieldlab Adapter #1287
Add Yieldlab Adapter #1287
Conversation
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.
@mirkorean thank you for submitting your adapter. I'm the first reviewer and can tell your adapter looks pretty solid. I have some feedback on the source code and in the test case front, where we try to keep the coverage above 90%, I believe a couple more test cases to cover some more scenarios in adapters/yieldlabyieldlab.go
╭─ ~/go/src/github.com/prebid/prebid-server/adapters/yieldlab ‹PR1287›
╰─➤ go test -coverprofile=c.out
PASS
coverage: 83.4% of statements
ok github.com/prebid/prebid-server/adapters/yieldlab 0.035s
╭─ ~/go/src/github.com/prebid/prebid-server/adapters/yieldlab ‹PR1287*›
╰─➤ go tool cover -func=c.out
github.com/prebid/prebid-server/adapters/yieldlab/usersync.go:10: NewYieldlabSyncer 100.0%
github.com/prebid/prebid-server/adapters/yieldlab/yieldlab.go:28: NewYieldlabBidder 0.0%
github.com/prebid/prebid-server/adapters/yieldlab/yieldlab.go:37: MakeRequests 100.0%
github.com/prebid/prebid-server/adapters/yieldlab/yieldlab.go:51: makeEndpointURL 86.7%
github.com/prebid/prebid-server/adapters/yieldlab/yieldlab.go:101: getGDPR 85.7%
github.com/prebid/prebid-server/adapters/yieldlab/yieldlab.go:125: makeTargetingValues 100.0%
github.com/prebid/prebid-server/adapters/yieldlab/yieldlab.go:133: makeRequest 84.2%
github.com/prebid/prebid-server/adapters/yieldlab/yieldlab.go:170: parseRequest 80.0%
github.com/prebid/prebid-server/adapters/yieldlab/yieldlab.go:194: mergeParams 100.0%
github.com/prebid/prebid-server/adapters/yieldlab/yieldlab.go:212: MakeBids 77.8%
github.com/prebid/prebid-server/adapters/yieldlab/yieldlab.go:281: findBidReq 80.0%
github.com/prebid/prebid-server/adapters/yieldlab/yieldlab.go:292: makeBannerAdSource 100.0%
github.com/prebid/prebid-server/adapters/yieldlab/yieldlab.go:296: makeAdSourceURL 100.0%
github.com/prebid/prebid-server/adapters/yieldlab/yieldlab.go:315: makeCreativeID 100.0%
github.com/prebid/prebid-server/adapters/yieldlab/yieldlab.go:319: splitSize 70.0%
total: (statements) 85.1%
Thank you for the review @guscarreon! Much Appreciated! I'll let @aklinkert respond to your comments as he did all the work on the adapter. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Commenting to remove the stale label while we're waiting on @aklinkert. |
@guscarreon @mirkorean I updated the code (rebased on current master) and modified it to reflect the suggestions by @guscarreon. Regarding coverage: I did some lower hanging fruits to increase the test coverage, anything else would result in unjustified increase in amount of test code. I looked through the coverage report, all uncovered lines are now error handling cases and IMHO do not need to be tested explicitly. We now have a coverage of 87.4% and by thus are in the upper third, looking though the other adapters coverage. |
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.
LGTM
Signed-off-by: Alex Klinkert <alex@klinkert.io>
Co-authored-by: Mirko Feddern <mirkorean@users.noreply.github.com> Signed-off-by: Alex Klinkert <alex@klinkert.io>
@SyntaxNode Are we able to merge this PR? Or is there anything left that needs a second glance? :) |
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.
LGTM
New Adapter for Yieldlab (https://www.yieldlab.com)
Test Parameters:
https://github.com/prebid/Prebid.js/blob/master/modules/yieldlabBidAdapter.md
[x] Official
Contact: solutions@yieldlab.de