-
Notifications
You must be signed in to change notification settings - Fork 748
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
inherit bidder params from parent for an alias #3048
Conversation
We no longer require to maintain aliasBidderNames in separate slice. Instead, we will be needing aliasBidderToParentBidder map to maintain so that we can use it to get parent schema in later commit.
This refactor makes it easier to test the NewBidderParamsValidator function. Also adds the test cases.
openrtb_ext/bidders.go
Outdated
var paramsValidator paramsValidatorHelper = paramsHelper{} | ||
|
||
// NewBidderParamsValidator makes a BidderParamValidator, assuming all the necessary files exist in the filesystem. | ||
// This will error if, for example, a Bidder gets added but no JSON schema is written for them. | ||
func NewBidderParamsValidator(schemaDirectory string) (BidderParamValidator, error) { | ||
fileInfos, err := os.ReadDir(schemaDirectory) | ||
fileInfos, err := paramsValidator.readDir(schemaDirectory) |
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.
Instead of declaring paramsValidator
as package/global var. Update NewBidderParamsValidator
to take paramsValidatorHelper
interface as input.
Callers of NewBidderParamsValidator
can then pass paramsValidator
or mockParamsHelper
as implementor
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.
Well, in that case we would need to make paramsValidatorHelper
interface and paramsHelper
struct public since caller of the NewBidderParamsValidator
function is outside this package. IMO, caller should not worry about the implementation details of NewBidderParamsValidator
function and should be handled within the package itself.
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.
few suggestions
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.
Looking good. Mostly just nitpicks.
Currently, we have adapter aliases that defines Bidder params explicitly using json file. However, after the adapter aliases feature, we will have the bidder params inherited from its parent adapter.
To accomplish this, we had to do some refactoring and remove the unnecessary code. We also refactored NewBidderParamsValidator function so that we can write the test cases.