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.

Sign up for GitHub

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

Jump to bottom

Add JSDoc for pbjs.getAllWinningBids #1566

Merged
merged 3 commits into from
Oct 3, 2017
Merged

Add JSDoc for pbjs.getAllWinningBids #1566

merged 3 commits into from
Oct 3, 2017

Conversation

rmloveland
Copy link
Contributor

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
  • Other

Description of change

Add JSDoc for pbjs.getAllWinningBids.

Other information

Relates to #1408

@protonate protonate requested a review from dbemiller September 7, 2017 19:17
dbemiller
dbemiller approved these changes Sep 7, 2017
View reviewed changes
Copy link
Contributor

@dbemiller dbemiller left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This looks fine, and feel free to merge it as is if you want, but..

Do you know which properties are on those bids? It would be really helpful to callers if we had some kind of:

/**
 * @typedef {Object} FinalBid
 * 
 * @property ...
 * @property ...
 */

somewhere, and then use @return {FinalBid[]} here

Add a Bid type; update return value
c375c4b 
This type will be useful in a few other places around the API docs (that
work will happen on separate PRs).
@rmloveland
Copy link
Contributor Author

Great idea Dave - I just pushed another update which adds a Bid type which can be used in the return value for this method (and a few others once this goes in).

@dbemiller
Copy link
Contributor

Awesome :). One small issue... since #1494 got merged, there's now a duplicate Bid type, with its own set of expectations. :/

The root of the issue is that the bid which we give publishers has more stuff on it than the bid which an Adapter returns.

I'm not sure what the best words are to describe the ideas here. We could rename the one in bidderFactory to AdapterBid, or rename the one you have here as FinalBid, or something like that. Do you have any preferences? Good naming is hard... and engineers suck at it :(.

dbemiller
dbemiller reviewed Sep 15, 2017
View reviewed changes
src/prebid.js
* @property {string} statusMessage The status of the bid. Allowed values: `"Bid available"` or `"Bid returned empty or error response"`.
* @property {number} cpm The exact bid price from the bidder, expressed to the thousandths place. Example: `"0.849"`.
*
* @property {Object} adserverTargeting An object whose values represent the ad server's targeting on the bid.
Copy link
Contributor

Choose a reason for hiding this comment

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

Your call, but... this might be worth a typedef of its own. There are a few getAdserverTargeting functions which (I think) return these same properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree but I think it might be better to do this in another PR for the sake of expediency.

dbemiller
dbemiller reviewed Sep 15, 2017
View reviewed changes
src/prebid.js
*
* @property {string} size The size of the ad creative, expressed in `"AxB"` format, where A and B are numbers of pixels. Example: `"320x50"`.
* @property {string} width The width of the ad creative in pixels. Example: `"320"`.
* @property {string} height The height of the ad creative in pixels. Example: `"50"`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these really strings, and not numbers? yuck!

@rmloveland
Copy link
Contributor Author

Just pushed a commit changing the type to AdapterBidResponse, for now. I think with some additional work on the code side we can merge the ever-so-slightly-different bid data structures and then document them with one typedef.

@rmloveland rmloveland mentioned this pull request Sep 20, 2017
Closed
@dbemiller
Copy link
Contributor

I think with some additional work on the code side we can merge the ever-so-slightly-different bid data structures and then document them with one typedef.

That's fine... but one word of caution on here: you could document every single object in the Prebid ecosystem if you pooled all the optional properties together, like so:

/**
 * @typedef {object} PrebidObject
 *
 * @param {number} [cpm] The value of the bid, if this object has a value.
 * @param {string} [code] An ID for the object, if relevant.
 * ...
*/

It would be accurate, since it documents all the properties which may or may not exist... but it would be totally unhelpful to both implementers and callers.

The main goal of JSDocs, and Types as a whole, is to align the expectations of the implementer and caller. So if the functions truly guarantee to return objects with different properties--even if most of them overlap--then you're probably better keeping the types separate.

@dbemiller dbemiller requested a review from protonate September 20, 2017 18:26
@dbemiller dbemiller assigned protonate and unassigned dbemiller Sep 20, 2017
@rmloveland
Copy link
Contributor Author

@protonate Do you mind taking a look at this one when you get a chance? Happy to make any updates as needed!

protonate
protonate approved these changes Oct 2, 2017
View reviewed changes
Copy link
Collaborator

@protonate protonate 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 To Me

I agree on targeting being it's own type, and that should be scoped separately.

I think dimensions as strings is carryover from the XHTML heresies requiring all attributes be quoted. HTML5 spec is that these dimensions are non-zero integers.

@matthewlane matthewlane merged commit 684ddf3 into master Oct 3, 2017
@matthewlane matthewlane deleted the docs-getAllWinningBids branch October 3, 2017 18:32
vzhukovsky added a commit to aol/Prebid.js that referenced this pull request Oct 25, 2017
@vzhukovsky
Merge pull request prebid#128 in AOLP_ADS_JS/prebid.js from release/1…
ae0895f 
….30.0 to aolgithub-master

* commit '5a8d2bf93ee15071a78e24ac976103cacf3c6021': (35 commits)
  Added changelog entry.
  Prebid 0.30.1 Release
  Remove undefined variable usage (prebid#1662)
  fixes bug for IE when invalid value passed to parse (prebid#1657)
  Aliasbidder fix (prebid#1652)
  prebidAdapter secure support (prebid#1655)
  Increment pre version
  Prebid 0.30.0 Release
  Add native param support to mediaTypes (prebid#1625)
  PulsePoint Lite adpater changes (prebid#1630)
  Appnexus ast unittest updates (prebid#1654)
  Support aspect ratio specification for native images (prebid#1634)
  Revert changes for switch between client side and server side. (prebid#1653)
  rubicon converted to bidderFactory (prebid#1624)
  Add JSDoc for `pbjs.getAllWinningBids` (prebid#1566)
  Add ignore-loader to handle .md files (prebid#1646)
  fixed PBS cookie syncs (prebid#1637)
  Add placementId request param to Yieldmo bid adapter (prebid#1632)
  Adxcg analytics adapter (prebid#1599)
  Add publisher sub-id support to the Criteo adapter (prebid#1629)
  ...
dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018
@rmloveland @dluxemburg
Add JSDoc for pbjs.getAllWinningBids (prebid#1566)
e091b79 
* Add JSDoc for `pbjs.getAllWinningBids`

* Add a `Bid` type; update return value

This type will be useful in a few other places around the API docs (that
work will happen on separate PRs).

* s/Bid/AdapterBidResponse/g
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbemiller dbemiller dbemiller approved these changes

@protonate protonate protonate approved these changes

Assignees

@protonate protonate

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rmloveland @dbemiller @protonate @matthewlane