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

SmartyadsBidAdapter: remove usprivacy #11829

Merged

Conversation

rishko00
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

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

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

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

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 do not attempt malicious changes designed to circumvent our rules

  • [

'language': (navigator && navigator.language) ? navigator.language : '',
'secure': 1,
'host': location?.domain ?? '',
'page': location?.page ?? '',
'coppa': config.getConfig('coppa') === true ? 1 : 0,
Copy link
Collaborator

@patmmccann patmmccann Jun 19, 2024

Choose a reason for hiding this comment

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

We will not be accepting tricks to fool the duplication check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@patmmccann OK, but how can I fix that? I saw the report and duplication check marks, for example, the isBidResponseValid function is used in lots of adapters and looks almost the same.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Check out this work in progress from an adapter structured very similar to yours

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@patmmccann I'm sorry, but I still don't get it. The previous version of the isBidResponseValid function was flagged as duplicate code by your tool, but it doesn't need any updates, right? And the same function with same code is used in other adapters

Copy link
Collaborator

@patmmccann patmmccann Jun 19, 2024

Choose a reason for hiding this comment

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

You and other modules have duplicate code, we need you to move common functions into libraries before making further updates to your adapter

Copy link
Collaborator

Choose a reason for hiding this comment

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

#8527 has an example on how to move common functions into a library

@patmmccann patmmccann self-assigned this Jun 19, 2024
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! 🚀

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

@rishko00
Copy link
Contributor Author

@patmmccann, I've moved some functions to the libs and removed some unused fields from the request and unused functions. However, there are still common functions, such as isBidResponseValid and interpretResponse. If I move isBidResponseValid to libs, there will still be an issue with duplicated code, but I can't rewrite it because it's used for common validations. Also I didn't find example of adapter where those functions are working properly. I need your advice on how to proceed.

@patmmccann
Copy link
Collaborator

Hi @rishko00 take a look at MaksymTeqBlaze@0249eb7#diff-a442c024395533115322fcbbfe796836b8ea37974565360dda15e9c93fbca181 or at #11849 to see examples of bidders merging common functions. The library function you added looks great but I am not sure if it would be imported in multiple adapters.

@patmmccann
Copy link
Collaborator

#11854 has merged in and is a great example

@rishko00
Copy link
Contributor Author

@patmmccann yes, got it, I've pushed changes

@patmmccann
Copy link
Collaborator

Looks great! Can you undo your changes to render files and we're good, thanks!!

@patmmccann patmmccann merged commit 1131a90 into prebid:master Jun 26, 2024
5 checks passed
@patmmccann patmmccann changed the title SmartyadsBidAdapter remove usprivacy SmartyadsBidAdapter: remove usprivacy Jun 26, 2024
DecayConstant pushed a commit to mediavine/Prebid.js that referenced this pull request Jul 18, 2024
* SmartyadsBidAdapter remove usprivacy

* refactor

* refactor

* remove unusable code

* try to fix duplicated code

* try to fix duplicated code

* smallfixes

* smallfixes

* smallfix

* tests fix

* revert fixes

* remove language

* move getAdUrlByRegion to libs

* refactor

* undo creative functions

---------

Co-authored-by: vrishko <vasyl.rishko@smartyads.com>
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.

3 participants