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

implements the todo comment in the code #4821

Merged
merged 3 commits into from
Feb 12, 2020
Merged

implements the todo comment in the code #4821

merged 3 commits into from
Feb 12, 2020

Conversation

patmmccann
Copy link
Collaborator

adUnit.sizes was still being referenced here in two places despite being deprecated. This simply deletes the references.

Type of change-

[ ] Bugfix

  • Feature
  • New 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

Removal of an object that is no longer needed

Other information

http://prebid.org/blog/pbjs-3

adUnit.sizes was still being referenced here in two places despite being deprecated. This simply deletes the references.
Fawke
Fawke previously approved these changes Feb 11, 2020
Copy link
Contributor

@Fawke Fawke left a comment

Choose a reason for hiding this comment

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

LGTM

@Fawke Fawke self-requested a review February 11, 2020 07:53
@Fawke Fawke dismissed their stale review February 11, 2020 07:53

Sorry, I found two unit test cases failing after approving this!

Copy link
Contributor

@Fawke Fawke left a comment

Choose a reason for hiding this comment

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

@patmmccann

Hi, thanks for this PR.

I believe there a two unit test cases which are failing in pbjs_api_spec.js in line: 1516 and line:1604 because we are still trying to assert against adUnit.sizes which you just removed in this PR.

Can you get rid of the assertions in those two line and then run the command, gulp test to validate all unit test cases are passing?

@patmmccann
Copy link
Collaborator Author

patmmccann commented Feb 11, 2020

I found several other assertions to delete as well based on adUnits[0].sizes

Copy link
Contributor

@Fawke Fawke left a comment

Choose a reason for hiding this comment

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

Thanks for making the change! LGTM.

@Fawke Fawke merged commit 469ec57 into prebid:master Feb 12, 2020
jsnellbaker added a commit that referenced this pull request Feb 13, 2020
jsnellbaker added a commit that referenced this pull request Feb 13, 2020
@patmmccann patmmccann deleted the patch-2 branch July 3, 2020 00:25
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.

3 participants