Skip to content

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

Closed
wants to merge 12 commits into from
Closed

Add Traffic #499

wants to merge 12 commits into from

Conversation

m1guelpf
Copy link
Contributor

@m1guelpf m1guelpf commented Jan 10, 2017

Fix #498
Add support for the traffic API.
https://developer.github.com/v3/repos/traffic/

@Nyholm
Copy link
Collaborator

Nyholm commented Jan 11, 2017

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.

@m1guelpf
Copy link
Contributor Author

m1guelpf commented Jan 11, 2017

@Nyholm I have looked in the tests, but I haven't seen a way to make tests. I am not familiar with tests, sorry 😢
Anyway, you merged my other PR (#489) without providing tests (I later did in #490)

@Nyholm
Copy link
Collaborator

Nyholm commented Jan 11, 2017

No worries. I'll help you out ~~~tonight when I get back from work~~~ tomorrow, I left my computer at the office.

Yes, merging #489 without tests was a mistake of mine. I'm happy you corrected my mistake in #490.

@m1guelpf
Copy link
Contributor Author

@Nyholm Thank you!

@m1guelpf
Copy link
Contributor Author

m1guelpf commented Jan 11, 2017

Fixed everything and checked it works!
Screenshot
(Click it to view it full-size)

@m1guelpf m1guelpf mentioned this pull request Jan 11, 2017
*
* @return array
*/
public function views($owner, $repository)
Copy link
Collaborator

Choose a reason for hiding this comment

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

*
* @return array
*/
public function clones($owner, $repository)
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.

Done

Copy link
Collaborator

Choose a reason for hiding this comment

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

Still not added.

@Nyholm
Copy link
Collaborator

Nyholm commented Jan 12, 2017

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?

@m1guelpf
Copy link
Contributor Author

@Nyholm Replied in your PR.

Copy link
Collaborator

@Nyholm Nyholm left a 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
{
// ...
Copy link
Collaborator

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()
Copy link
Collaborator

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

Copy link
Collaborator

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

@m1guelpf
Copy link
Contributor Author

m1guelpf commented Feb 1, 2017

@Nyholm As I said earier, I don't know hot to write tests...

@Nyholm
Copy link
Collaborator

Nyholm commented Feb 1, 2017

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

Copy link
Collaborator

@Nyholm Nyholm left a 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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Still not added.

@Nyholm Nyholm mentioned this pull request Mar 23, 2017
@m1guelpf
Copy link
Contributor Author

@Nyholm Oops! I deleted the repo...
Is there any way I can recover it?

@Nyholm
Copy link
Collaborator

Nyholm commented Mar 23, 2017

I do not think so. Sorry.

@Nyholm Nyholm added this to the Release 2.2 milestone Mar 24, 2017
@Nyholm
Copy link
Collaborator

Nyholm commented Mar 24, 2017

I added this to the milestone 2.2. Is it possible that you can add tests to this PR this weekend?

@m1guelpf
Copy link
Contributor Author

@Nyholm I'll try

@m1guelpf
Copy link
Contributor Author

@Nyholm Moved to #548

@m1guelpf m1guelpf closed this Mar 26, 2017
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