-
-
Notifications
You must be signed in to change notification settings - Fork 600
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
Events list per authenticated user for all repos #1000
Events list per authenticated user for all repos #1000
Conversation
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.
Can you also add a unit test to also cover the behaviour there? Thanks!
Example:
php-github-api/test/Github/Tests/Api/UserTest.php
Lines 23 to 37 in 26a838b
/** | |
* @test | |
*/ | |
public function shouldShowByIdUser() | |
{ | |
$expectedArray = ['id' => 1, 'username' => 'l3l0']; | |
$api = $this->getApiMock(); | |
$api->expects($this->once()) | |
->method('get') | |
->with('/user/1') | |
->will($this->returnValue($expectedArray)); | |
$this->assertEquals($expectedArray, $api->showById(1)); | |
} |
lib/Github/Api/User.php
Outdated
* | ||
* @return array | ||
*/ | ||
public function events($username, $page = 1, $perPage = 30) |
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 remove the $page
and $perPage
parameters, the ResultPager
should be used to loop over all the results
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 just removed both parameters, thank you.
lib/Github/Api/User.php
Outdated
* | ||
* @return array | ||
*/ | ||
public function events($username, $page = 1, $perPage = 30) |
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.
Also add the string
typehint to the $username
parameter and remove the superfluous @param
docs
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 added the typehint and I removed completely the @param docs. I also would remove the complete block of comments, since the function looks pretty readable. But I left it to keep consistency with previous methods.
@acrobat thank you for reviewing and suggesting. I submitted the changes. |
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.
Can you just add a unit test to also cover the behaviour there? Thanks!
Example:
php-github-api/test/Github/Tests/Api/UserTest.php
Lines 23 to 37 in 26a838b
/** | |
* @test | |
*/ | |
public function shouldShowByIdUser() | |
{ | |
$expectedArray = ['id' => 1, 'username' => 'l3l0']; | |
$api = $this->getApiMock(); | |
$api->expects($this->once()) | |
->method('get') | |
->with('/user/1') | |
->will($this->returnValue($expectedArray)); | |
$this->assertEquals($expectedArray, $api->showById(1)); | |
} |
Otherwise the PR is good to go!
@acrobat yes sorry I did it yesterday but I didn't added the file. |
No problem @richard015ar, thanks! And congrats on your first contribution! 🎉 |
This change add a list of events for all repos for an authenticated user: https://docs.github.com/en/rest/reference/activity#list-events-for-the-authenticated-user.
It is pretty useful if you want to get a list of different types of events without specify any repo.