-
Notifications
You must be signed in to change notification settings - Fork 2k
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
gptPreAuction: pass publisher provided signals to GPT #11946
Conversation
Whoa there, partner! 🌵🤠 We wrangled some duplicated code in your PR:
Reducing code duplication by importing common functions from a library not only makes our code cleaner but also easier to maintain. Please move the common code from both files into a library and import it in each. We hate that we have to mention this, however, commits designed to hide from this utility by renaming variables or reordering an object are poor conduct. We will not look upon them kindly! Keep up the great work! 🚀 |
Tread carefully! This PR adds 1 linter warning (possibly disabled through directives):
|
src/pps.js
Outdated
} | ||
|
||
export function getSignalsIntersection(signals) { | ||
const taxonomies = ['IAB_AUDIENCE_1_1', 'IAB_CONTENT_2_2']; |
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.
these can be extracted to be reused with getSignals
above
src/pps.js
Outdated
export function getAuctionsIdsFromTargeting(targeting) { | ||
return Object.values(targeting) | ||
.flatMap(x => Object.entries(x)) | ||
.filter((entry) => (entry[0] || '').includes(TARGETING_KEYS.AD_ID)) |
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.
I think this should be entry[0] === TARGETING_KEYS.AD_ID || entry[0].startsWith(TARGETING_KEYS.AD_ID + '_')
That's because
- I don't think object keys can be falsy (afaik the language requires them to be either strings or symbols), so you don't need the fallback to
''
- You are interested in either
hb_adid
orhb_adid_${bidder}
(reference). It's unlikely, but possible, to define custom targeting keys likecustom_hb_adid
that would be a false positive for this.
src/pps.js
Outdated
return signals; | ||
} | ||
|
||
export function getSignalsArrayByAuctionsIds(auctionIds) { |
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.
(regarding the test failure) my preferred method of stubbing is dependency injection. E.g. if this did something like getSignalsArrayByAuctionIds(auctionIds, index = auctionManager.index)
, tests can swap in a mock version of index
, and I find it a bit easier to reason about compared to sinon's stubs. To do the same with sinon I think you need an extra layer of indirection, basically instead of making index
a parameter make it a property of some global object pointing to auctionManager.index
, which you can then stub.
src/pps.js
Outdated
.flatMap(x => Object.entries(x)) | ||
.filter((entry) => (entry[0] || '').includes(TARGETING_KEYS.AD_ID)) | ||
.flatMap(entry => entry[1]) | ||
.filter(uniques); |
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.
note that these are ad ids, not auction IDs. they should be passed through findBidByAdId
.
Whoa there, partner! 🌵🤠 We wrangled some duplicated code in your PR:
Reducing code duplication by importing common functions from a library not only makes our code cleaner but also easier to maintain. Please move the common code from both files into a library and import it in each. We hate that we have to mention this, however, commits designed to hide from this utility by renaming variables or reordering an object are poor conduct. We will not look upon them kindly! Keep up the great work! 🚀 |
Tread carefully! This PR adds 2 linter errors and 1 linter warning (possibly disabled through directives):
|
Whoa there, partner! 🌵🤠 We wrangled some duplicated code in your PR:
Reducing code duplication by importing common functions from a library not only makes our code cleaner but also easier to maintain. Please move the common code from both files into a library and import it in each. We hate that we have to mention this, however, commits designed to hide from this utility by renaming variables or reordering an object are poor conduct. We will not look upon them kindly! Keep up the great work! 🚀 |
Tread carefully! This PR adds 2 linter errors and 1 linter warning (possibly disabled through directives):
|
Whoa there, partner! 🌵🤠 We wrangled some duplicated code in your PR:
Reducing code duplication by importing common functions from a library not only makes our code cleaner but also easier to maintain. Please move the common code from both files into a library and import it in each. We hate that we have to mention this, however, commits designed to hide from this utility by renaming variables or reordering an object are poor conduct. We will not look upon them kindly! Keep up the great work! 🚀 |
Tread carefully! This PR adds 1 linter error and 1 linter warning (possibly disabled through directives):
|
Tread carefully! This PR adds 1 linter warning (possibly disabled through directives):
|
Whoa there, partner! 🌵🤠 We wrangled some duplicated code in your PR:
Reducing code duplication by importing common functions from a library not only makes our code cleaner but also easier to maintain. Please move the common code from both files into a library and import it in each. We hate that we have to mention this, however, commits designed to hide from this utility by renaming variables or reordering an object are poor conduct. We will not look upon them kindly! Keep up the great work! 🚀 |
Tread carefully! This PR adds 1 linter warning (possibly disabled through directives):
|
Tread carefully! This PR adds 1 linter warning (possibly disabled through directives):
|
Tread carefully! This PR adds 1 linter warning (possibly disabled through directives):
|
* 10997 set pps to gam display * update * update * review changes * module handling * code sharing * linting fixes * Rename setPPSConfig * Filter out adIds that have no auction * use eql instead of JSON for deep equals --------- Co-authored-by: Marcin Komorski <[email protected]> Co-authored-by: Demetrio Girardi <[email protected]>
* 10997 set pps to gam display * update * update * review changes * module handling * code sharing * linting fixes * Rename setPPSConfig * Filter out adIds that have no auction * use eql instead of JSON for deep equals --------- Co-authored-by: Marcin Komorski <[email protected]> Co-authored-by: Demetrio Girardi <[email protected]>
Type of change
Bugfix
Feature
New bidder adapter
Updated bidder adapter
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Does this change affect user-facing APIs or examples documented on http://prebid.org?
Other
Description of change
Other information
#10997