-
Notifications
You must be signed in to change notification settings - Fork 758
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 LockerDome bid adapter #1026
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.
LGTM in general. Just some minor comments
openrtb_ext/imp_lockerdome.go * Delete unnecessary file adapters/lockerdome/lockerdome.go * Better error message for unmarshaling LockerDome bid response * Inline bidsCapacity * Move const unexpectedStatusCodeMessage to top adapters/lockerdome/lockerdometest/exemplary/simple-banner.json * Better error message for unmarshaling LockerDome bid response adapters/lockerdome/lockerdometest/exemplary/simple-banner-multiple.json * Add test case w/ multiple banner requests config/config.go * Inline lockerDomePlatformID
Thank you for your feedback, I've just pushed up changes. |
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.
Validation of all elements in openRTBRequest.Imp[]
is needed. Try to keep coverage high after validation is added, 93 to 95 percent is pretty good.
╰─➤ go test -coverprofile=c.out
PASS
coverage: 93.3% of statements
ok github.com/prebid/prebid-server/adapters/lockerdome 0.029s
╰─➤ go tool cover -func=c.out
github.com/prebid/prebid-server/adapters/lockerdome/lockerdome.go:21: MakeRequests 81.8%
github.com/prebid/prebid-server/adapters/lockerdome/lockerdome.go:54: MakeBids 100.0%
github.com/prebid/prebid-server/adapters/lockerdome/lockerdome.go:94: NewLockerDomeBidder 100.0%
github.com/prebid/prebid-server/adapters/lockerdome/usersync.go:10: NewLockerDomeSyncer 100.0%
total: (statements) 93.3%
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.
Please let me know if my latest comment is required or desirable on your end. If not, I'm perfectly ok with approving.
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.
One minor optimization comment. Appart from that, looks pretty good
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 great Margaret. Thank you for your patience.
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. |
Yay! Thank you! |
No description provided.