-
Notifications
You must be signed in to change notification settings - Fork 750
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
Bidder Info Endpoint: Base Adapter Only Filter #3088
Conversation
endpoints/info/bidders_test.go
Outdated
expected: `["a","z"]`, | ||
}, | ||
{ | ||
name: "defaultequest-one", |
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.
Should this and the below be defaultrequest
rather than defaultequest
?
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. Yes it should. Nice catch. Fixed.
@@ -94,3 +138,32 @@ func prepareBiddersResponseEnabledOnly(bidders config.BidderInfos, aliases map[s | |||
|
|||
return json.Marshal(bidderNames) | |||
} | |||
|
|||
func prepareBiddersResponseEnabledOnlyBaseOnly(bidders config.BidderInfos) ([]byte, error) { |
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.
Does this function need the word Only in it twice? Would prepareBiddersResponseEnabledBaseOnly
be better? Or is it better to clearly state that we're looking for only endpoint Enabled and only Base adapters?
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 don't think it matters too much. The names are intentional to match up with the EnabledOnly
and BaseOnly
flags.
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.
LGTM
Per: #3079