-
-
Notifications
You must be signed in to change notification settings - Fork 597
Add Traffic #499
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
Add Traffic #499
Conversation
Excellent. Thank you for this PR. I see that it is missing some tests. Would you add some and I'll do a review of this. |
@Nyholm Thank you! |
[ci skip] [skip ci]
* | ||
* @return array | ||
*/ | ||
public function views($owner, $repository) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing parameter per
. See https://developer.github.com/v3/repos/traffic/#views
* | ||
* @return array | ||
*/ | ||
public function clones($owner, $repository) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing parameter per
. See https://developer.github.com/v3/repos/traffic/#clones
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still not added.
I've created a PR to explain how to write tests. See #503 Is it understandable? Should I add something to it so it makes more sense? |
[ci skip] [skip ci]
@Nyholm Replied in your PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this PR. Just make sure to add the per
parameters and write some proper tests and I'll be happy to merge.
|
||
class TrafficTest extends TestCase | ||
{ | ||
// ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be removed
/** | ||
* @test | ||
*/ | ||
public function shoulddoSomething() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the name of this function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should also update the test content
@Nyholm As I said earier, I don't know hot to write tests... |
Tests specific to this library or in general? Read about how to write tests to the Github library here: https://github.com/KnpLabs/php-github-api/blob/master/doc/testing.md Testing in general: https://www.startutorial.com/articles/view/phpunit-beginner-part-1-get-started |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot merge this without tests. Will you work on the tests?
* | ||
* @return array | ||
*/ | ||
public function clones($owner, $repository) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still not added.
@Nyholm Oops! I deleted the repo... |
I do not think so. Sorry. |
I added this to the milestone 2.2. Is it possible that you can add tests to this PR this weekend? |
@Nyholm I'll try |
Fix #498
Add support for the traffic API.
https://developer.github.com/v3/repos/traffic/