-
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
ttd bid adapter: configurable endpoint #12004
ttd bid adapter: configurable endpoint #12004
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! 🚀 |
Hi @tongwu-sh ? It seems your adapter needs a tiny bit of additional maintenance: please move the common get connection logic in your adapter and prisma into a file in /libraries It seems you also have a bug in your battr support; I think it is on imp.banner instead of imp Please also document this endpoint change on the docs repo |
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.
Couple of minor requests
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! 🚀 |
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 (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 26 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 26 linter errors and 1 linter warning (possibly disabled through directives):
|
Thanks for the comments; I've made the updates. How should we test this part? |
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! 🚀 |
This looks good to me, if you pull in master you'll pass that viant test. I noticed you marked as draft so holding off on merge Thanks! |
switch (connection.type) { | ||
case 'ethernet': | ||
return 1; | ||
case 'wifi': |
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.
Do we want to include "wimax" with this case?
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.
Yes, added. And checked for chrome/edge looks like connection.type is not always there but connection.effectiveType has value. Updated to handle that case.
Fixed, #12011 |
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! 🚀 |
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! 🚀 |
@patmmccann , we finished the test for this change, could you help merge it? Thanks! |
Hello @patmmccann , we finished tests for this change and ready to merge, could you help take another look at this PR? Thanks! |
Type of change
Description of change
We want to allow users to specify their preferred TTD endpoint instead of the default endpoint for bidding.
Be sure to test the integration with your adserver using the Hello World sample page. -->
Other information