Skip to content
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

New Adapter: yahoossp (Rebrand yssp to yahoossp keeping the yssp alias) #1994

Merged
merged 2 commits into from
Sep 23, 2021

Conversation

oath-jac
Copy link
Contributor

We were told by legal that we'll go further with yahoossp name. The verizonmedia adapter will be removed at later stage.

@SyntaxNode SyntaxNode changed the title Rebrand yssp to yahoossp keeping the yssp alias. New Adapter: yahoossp (Rebrand yssp to yahoossp keeping the yssp alias) Sep 13, 2021
Copy link
Contributor

@VeronikaSolovei9 VeronikaSolovei9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@guscarreon guscarreon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good but before approving I was wondering if we still need files openrtb_ext/imp_yssp.go, static/bidder-params/yssp.json and static/bidder-info/yssp.yaml? We seem to no longer be using the ExtImpYSSP struct inside adapters/yahoossp/yahoossp.go and ExtImpYahooSSP seems to be a identical to ExtImpYSSP except for the name.

1 package openrtb_ext
2
3 // ExtImpYSSP defines the contract for bidrequest.imp[i].ext.yssp
4 type ExtImpYSSP struct {
5     Dcn string `json:"dcn"`
6     Pos string `json:"pos"`
7 }
openrtb_ext/imp_yssp.go

static/bidder-info/yssp.yaml and static/bidder-info/yahoossp.yaml seem to be identical files too.

exchange/adapter_builders.go Show resolved Hide resolved
openrtb_ext/bidders.go Show resolved Hide resolved
openrtb_ext/bidders.go Show resolved Hide resolved
config/config.go Show resolved Hide resolved
@oath-jac
Copy link
Contributor Author

We wanted to keep the yssp alias for the yahoossp adapter, that means we declared the bidder using the same builder.
We can not remove the static/bidder-info/yssp.yaml as it is needed.
In go server I understood that for aliases you reuse the same bidder builder but you have to duplicate some files like the static ones. In the java server we just declare the alias in the server config aliases property.

Looks good but before approving I was wondering if we still need files openrtb_ext/imp_yssp.go, static/bidder-params/yssp.json and static/bidder-info/yssp.yaml? We seem to no longer be using the ExtImpYSSP struct inside adapters/yahoossp/yahoossp.go and ExtImpYahooSSP seems to be a identical to ExtImpYSSP except for the name.

1 package openrtb_ext
2
3 // ExtImpYSSP defines the contract for bidrequest.imp[i].ext.yssp
4 type ExtImpYSSP struct {
5     Dcn string `json:"dcn"`
6     Pos string `json:"pos"`
7 }
openrtb_ext/imp_yssp.go

static/bidder-info/yssp.yaml and static/bidder-info/yahoossp.yaml seem to be identical files too.

We wanted to keep the yssp alias for the yahoossp adapter, that means we declared the bidder using the same builder.
We can not remove the static/bidder-info/yssp.yaml as it is needed.
In go server I understood that for aliases you reuse the same bidder builder but you have to duplicate some files like the static ones. In the java server we just declare the alias in the server config aliases property.

@SyntaxNode
Copy link
Contributor

Looks good to me. Please remove the ExtImpYSSP as the intent is for the ExtImpXXX structs to correlate to an adapter implementation, and now you have - rightfully so - ExtImpYahooSSP.

@SyntaxNode
Copy link
Contributor

In the java server we just declare the alias in the server config aliases property.

The alias functionality in PBS-Go is still in its infancy. We'll continue to make improvements in the near future. Don't expect to see the exact same approach from PBS-Java, we have other ideas in mind. The end goal is similar though in that no code is required for aliases.

@SyntaxNode
Copy link
Contributor

Please also remove the /params/race/banner.json file. That race detection pattern has been removed and replaced with enhanced memory sharing checks in the json test framework.

Copy link
Contributor

@guscarreon guscarreon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@SyntaxNode
Copy link
Contributor

@oath-jac This is looking good. Please open a docs PR for the new yahoossp bidder, just as you did for yssp previously. We will not merge this PR until the docs PR is opened.

Copy link
Contributor

@VeronikaSolovei9 VeronikaSolovei9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@SyntaxNode
Copy link
Contributor

Docs PR: prebid/prebid.github.io#3283

@SyntaxNode SyntaxNode merged commit 657232b into prebid:master Sep 23, 2021
jizeyopera pushed a commit to operaads/prebid-server that referenced this pull request Oct 13, 2021
…s) (prebid#1994)

Co-authored-by: oath-jac <dsp-supply-prebid@verizonmedia.com>
shunj-nb pushed a commit to ParticleMedia/prebid-server that referenced this pull request Nov 8, 2022
…s) (prebid#1994)

Co-authored-by: oath-jac <dsp-supply-prebid@verizonmedia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants