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

ZetaGlobalSsp adapter: merge ortb2.site and params.site #11773

Merged

Conversation

asurovenko-zeta
Copy link
Contributor

Type of change

  • Feature
    Merge params.site and ortb2.site in payload.site

asurovenko-zeta and others added 30 commits March 24, 2021 14:47
Copy link

Whoa there, partner! 🌵🤠 We wrangled some duplicated code in your PR:

  • modules/zeta_global_sspBidAdapter.js has 32 duplicated lines with modules/zmaticooBidAdapter.js
  • modules/zeta_global_sspBidAdapter.js has 15 duplicated lines with modules/zmaticooBidAdapter.js
  • modules/zeta_global_sspBidAdapter.js has 10 duplicated lines with modules/zmaticooBidAdapter.js
  • modules/zeta_global_sspBidAdapter.js has 23 duplicated lines with modules/zmaticooBidAdapter.js
  • modules/zeta_global_sspBidAdapter.js has 24 duplicated lines with modules/zmaticooBidAdapter.js
  • modules/adtrueBidAdapter.js has 13 duplicated lines with modules/zeta_global_sspBidAdapter.js

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

Looks to be failing tests and requiring several code block imports to solve duplication with zmaticoo

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

@asurovenko-zeta
Copy link
Contributor Author

@patmmccann thanks for review. That issue has been fixed

@patmmccann
Copy link
Collaborator

Doesn't look fixed, this adapter and zmatico should have a bunch of common imports committed

@surovenko
Copy link
Contributor

@patmmccann still didn't understand the issue here. Should I change not our zmaticoo Adapter? How to know what exactly lines should I move to common methods? To where? Do you have any example of this? First time saw the issue like this
thanks!

@patmmccann
Copy link
Collaborator

#8527 demonstrates moving common code into the library folder. #11831 is an example moving something to utils

@patmmccann patmmccann self-assigned this Jun 20, 2024
@patmmccann patmmccann self-requested a review June 20, 2024 21:38
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! 🚀

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

@surovenko
Copy link
Contributor

@patmmccann
Our adapter was simply copied and I cannot reasonably avoid this Github warning.
I tried to start moving methods into the utility class. But first of all, to avoid this warning I will have to copy everything from adapter. Secondly, I expect in the utility class only functions that can be used by everyone, some general functions that do not belong to some adapter, but some general things. But since our adapter was copied almost completely, we will have to transfer functions from our adapter that do not fit the definition of utility classes.
I don't know what to do in this situation. On the one hand, I want to remove the Github warning, on the other hand, it will be extremely difficult to remove it if I have 2 almost completely identical 2 files.

@patmmccann
Copy link
Collaborator

Are you not affiliated with zmatico? Have you seen the example in #11854 in which 20+ roughly identical adapters moved their functions into a library?

@patmmccann
Copy link
Collaborator

Your comments are quite reasonable, we're thinking about what to do still

@patmmccann patmmccann merged commit c1fbae1 into prebid:master Jun 28, 2024
4 of 5 checks passed
@patmmccann
Copy link
Collaborator

patmmccann commented Jun 30, 2024

One option, your adapter simply needs many updates: almost everything you take in a param needs the treatment of this pr, including the user object especially. Your usersync and constants block can reasonably be moved to /libraries/zetaUtils and we can fix zmaticoo to import them. Also I think this pr makes some of your site and device handling logic redundant or never reached, eg navigator.useragent

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.

None yet

3 participants