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

Outbrain adapter: send placement and plcmt fields separately #11799

Merged

Conversation

markkuhar
Copy link
Contributor

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

We would like to read both fields

Other information

Copy link

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! 🚀

@patmmccann
Copy link
Collaborator

Hi Outbrain team!

It appears your adapter shares several common code blocks with other adapters. Please import them from a library or constants definition table. Thanks!

Copy link
Collaborator

@patmmccann patmmccann left a comment

Choose a reason for hiding this comment

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

Please clean up the duplication issues

Copy link

github-actions bot commented Jul 1, 2024

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! 🚀

@patmmccann
Copy link
Collaborator

You code duplication generator started checking more adapters with your last commit, you can ignore pairs without your adapter, eg adform to dianomi

@patmmccann
Copy link
Collaborator

patmmccann commented Jul 1, 2024

Thanks for moving setonany, it seems only your tests aren't passing and this will be ready to merge when it is. Keep to the great work to get the code duplication solved on your next pr or feel free to finish it here. Looks like transformSizes is ripe for a move or a refactor to use existing patterns

@patmmccann patmmccann changed the title Oubtrain adapter: send placement and plcmt fields separately Outbrain adapter: send placement and plcmt fields separately Jul 1, 2024
@@ -390,7 +390,8 @@ describe('Outbrain Adapter', function () {
minduration: 3,
maxduration: 10,
startdelay: 2,
placement: 4,
placement: 5,
plcmt: 4,
linearity: 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you simply ordered these two lines differently than your tests expected

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, it should be ok like this. Locally all the tests pass, I am really not sure what is the problem. Can you see excected vs. actual diff on circle ci?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It passed, if you print it to console somehow you can, but I'm sure others have a better way

Copy link

github-actions bot commented Jul 1, 2024

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! 🚀

@patmmccann
Copy link
Collaborator

Btw did you see #11748

@patmmccann patmmccann merged commit 3192ed1 into prebid:master Jul 1, 2024
3 of 5 checks passed
DecayConstant pushed a commit to mediavine/Prebid.js that referenced this pull request Jul 18, 2024
…11799)

* start sending placement and placmt separately

* move setOnAny function to utils

* remove transformSizes and use util function
mefjush pushed a commit to adhese/Prebid.js that referenced this pull request Jul 19, 2024
…11799)

* start sending placement and placmt separately

* move setOnAny function to utils

* remove transformSizes and use util function
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants