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

Updated documentaion #1160

Merged
merged 3 commits into from
May 18, 2017
Merged

Updated documentaion #1160

merged 3 commits into from
May 18, 2017

Conversation

dbemiller
Copy link
Contributor

Type of change

  • Other (documentation)

Description of change

Removed some duplicate docs by linking the .md files together. Added a link to Karma, which is part of the testing tech stack.

More controversially: Deleted a chunk of docs which... honestly just made my eyes glaze over when I read them. I would place "how to fix unit tests" and "how to use a code coverage report" a bit outside of the scope of Prebid. Even Istanbul just links you to a sample report without any more real explanation.

README.md Outdated

### Additional Resources

- [Prebid Examples](http://prebid.org/dev-docs/getting-started.html)
Copy link
Member

Choose a reason for hiding this comment

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

I think this link belongs up near the top of the readme - since the consumer is anyone that is trying to implement prebid.js.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah... I agree, this does make much more sense

CONTRIBUTING.md Outdated

## Pull Requests
Please make sure that pull requests are scoped to one change, and that any added or changed code includes tests with greater than 80% code coverage. See [Testing Prebid.js](#testing-prebidjs) for help on writing tests.
Details about the Pull Request process can be found [here](./pr_review.md).
Copy link
Member

Choose a reason for hiding this comment

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

hate to have duplicative information, but I feel like stating 80% code coverage is nice to have here, even if it is in the linked doc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

- You may need to iterate on your code or tests until all tests are passing.
- Make sure existing tests still pass.
- There is a table below the testing report that shows code coverage percentage, for each file under the `src` directory.
- Each time you run tests, a code coverage report is generated in `build/coverage/lcov/lcov-report/index.html`.
Copy link
Member

Choose a reason for hiding this comment

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

Think having a link to this report is good to preserve, maybe just add a heading for Test Coverage

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. It's included under "Writing Tests" though.

My thought process was basically "the only time I care about code coverage is when I check to make sure my tests are good anyway."

@mkendall07 mkendall07 merged commit 1803b5a into master May 18, 2017
@mkendall07 mkendall07 deleted the improvement/update-readme branch May 19, 2017 00:43
outoftime pushed a commit to Genius/Prebid.js that referenced this pull request May 24, 2017
…built

* 'master' of https://github.com/prebid/Prebid.js: (23 commits)
  Increment pre version
  Probed 0.24.0 Release
  Beachfront adapter - add ad unit size (prebid#1183)
  Thoughtleadr adapter - fix postMessage (prebid#1207)
  When prebid server issues a no-bid response, call addBidResponse for every adUnit requested (prebid#1204)
  Improvement/timeout xhr (prebid#1172)
  Add native support (prebid#1072)
  Improvement/alias queue (prebid#1156)
  Updated documentaion (prebid#1160)
  Improvement/prebid iframes amp pages (prebid#1119)
  Fixes prebid#1114 possible xss issue (prebid#1186)
  Allowed setTargetingForGPTAsync() to target specific ad unit codes. (prebid#1158)
  updated tag (prebid#1212)
  Common user-sync (prebid#1144)
  Rename secureCreatives file and lint (prebid#1203)
  HIRO Media: Remove batching mechanism and use AJAX instead of JSONP (prebid#1133)
  Add Support for DigiTrust in Rubicon Adapter (prebid#1201)
  Upgrade linters to ESLint with stricter code style (prebid#1111)
  Add dynamic bidfloor parameter to Smart Adserver Adapter (prebid#1194)
  Bug fix: bids served by secure creatives does not get pushed into _winningBids (prebid#1192)
  ...
vzhukovsky added a commit to aol/Prebid.js that referenced this pull request Jul 17, 2017
….23.0 to aolgithub-master

* commit '136fc37637749a764070c35c03e7e87a5c157947': (33 commits)
  Added changelog entry.
  Implemented passing key values feature.
  Update code to ESlint rules.
  Prebid 0.24.1 Release
  tests: drop ie9 browserstack test
  Audience Network: separate size from format (prebid#1218)
  Bugfix/target filtering api fix (prebid#1220)
  Map sponsor request param to endpoint param (prebid#1219)
  Increment pre version
  Probed 0.24.0 Release
  Beachfront adapter - add ad unit size (prebid#1183)
  Thoughtleadr adapter - fix postMessage (prebid#1207)
  When prebid server issues a no-bid response, call addBidResponse for every adUnit requested (prebid#1204)
  Improvement/timeout xhr (prebid#1172)
  Add native support (prebid#1072)
  Improvement/alias queue (prebid#1156)
  Updated documentaion (prebid#1160)
  Improvement/prebid iframes amp pages (prebid#1119)
  Fixes prebid#1114 possible xss issue (prebid#1186)
  Allowed setTargetingForGPTAsync() to target specific ad unit codes. (prebid#1158)
  ...
dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018
* Added some links, and reorganized the documentation a little bit. Removed some details which seemed superfluous to me.

* Updated headers.

* Spread the additional resources sections throughout the docs. Re-added some information about pull requests.
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.

2 participants