-
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
Visx bid adapter: import utilities and retrieve data from user on ortb2 #11860
Visx bid adapter: import utilities and retrieve data from user on ortb2 #11860
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. 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. 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. Keep up the great work! 🚀 |
4233436
to
1f8216f
Compare
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. 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. 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. Keep up the great work! 🚀 |
Hello, check for duplicated code prevents us to merge it. Touching the code of the other bid adapters doesnot make sense. Can you please help us? |
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. Keep up the great work! 🚀 |
…trieved-google-topics-signals-from-topics-api
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. 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. Keep up the great work! 🚀 |
src/utils.js
Outdated
@@ -1231,3 +1231,14 @@ export function hasNonSerializableProperty(obj, checkedObjects = new Set()) { | |||
} | |||
return false; | |||
} | |||
|
|||
export function getBidFromResponse(respItem, LOG_ERROR_MESS) { |
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.
can you put this in /libraries/processResponse/index.js instead? we don't need it in core utils
} | ||
}; | ||
} | ||
if (user) { |
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 cover this behavior in a test so you can be confident it will not break. Thanks!
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 add one test and use /libraries instead of utils for the moved function. Thanks!
Feel free to ignore non visx issues! |
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. Keep up the great work! 🚀 |
How can i ignore them? Merging is blocked because of this. |
I'm in charge of the merge, I can override |
…b2 (prebid#11860) * AF-3740 retrieved topics data from user on ortb2 * AF-3740 kept old user data * AF-3740 added some checks for user data * AF-3740 used mergeDeep for merging user objects * AF-3740 provided unit tests for ortb2 data * AF-3740 moved some functionality to utils and used for avoiding duplication * AF-3740 provided unit tests
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
Retrieved and sent user data (for getting topics data)
Other information