-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Illumin Bid Adapter: initial release #10375
Conversation
@mmoschovas @ChrisHuie |
modules/illuminBidAdapter.js
Outdated
} | ||
|
||
export function extractCID(params) { | ||
return params.cId || params.CID || params.cID || params.CId || params.cid || params.ciD || params.Cid || params.CiD; |
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.
Question regarding these three extract methods. Is there a reason we are checking multiple combinations when the params are spec'd out to be cId
, pId
and subDomain
? This seems unnecessary to me and throwing a warning during validation seems appropriate here if the property name is not correct
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.
@mmoschovas We are checking this for backward compatibility and minor typos of publishers.
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.
Maybe I am missing something. Is this not a new adapter? Im not sure where backwards compatibility is coming into play. I also understand the intent, I just do not understand why you are doing it considering it seems these are random alternative values and not comprehensive. Either way, I believe you should allow publishers who do not follow the specification to receive the error and not attempt to handle unknowns
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.
@mmoschovas
I've changed the extract method to support only the given properties.
Can we merge this?
Type of change
Description of change
Illumin Bid Adapter
maintainer email: [email protected]
Other information
prebid/prebid.github.io#4813