-
Notifications
You must be signed in to change notification settings - Fork 760
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
AMX Adapter: support native format #2220
Conversation
These changes looks good to me. However while I was reviewing and testing this PR I noticed you didn't specify required parameters for |
Thanks Veronika! We do support empty ext.amx. We allow passing the tagId parameter via a custom endpoint that we provide publishers, and adUnitId is not required--that's why we don't require or test for imp.ext.amx. |
adapters/amx/amx_test.go
Outdated
} | ||
|
||
assert.Len(t, bids.Bids, 1) | ||
assert.Equal(t, bids.Bids[0].BidType, test.bidType) |
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.
The order for this needs to be the other way round like:
assert.Equal(t, test.bidType, bids.Bids[0].BidType)
The assert function signature requires the expected value to be specified first.
Reference: https://pkg.go.dev/github.com/stretchr/testify/assert#Equal
LGTM except for the above minor comment. Also, please update the docs here: https://github.com/prebid/prebid.github.io/blob/master/dev-docs/bidders/amx.md to include |
Thanks @mansinahar -- I made the change to the test & opened this PR: prebid/prebid.github.io#3714 |
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.
Thank you for your answers. LGTM
Thanks @nickjacob. Looks good to me |
This PR adds support for native ads & removes unnecessary legacy code that was used for interpolating pixels into ad markup: