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

Events list per authenticated user for all repos #1000

Merged
merged 6 commits into from
Apr 23, 2021

Conversation

richard015ar
Copy link
Contributor

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.

@richard015ar richard015ar changed the title List events per authenticated user List events per authenticated user for all repos Apr 21, 2021
@richard015ar richard015ar changed the title List events per authenticated user for all repos Events list per authenticated user for all repos Apr 21, 2021
Copy link
Collaborator

@acrobat acrobat left a 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:

/**
* @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));
}

*
* @return array
*/
public function events($username, $page = 1, $perPage = 30)
Copy link
Collaborator

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

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 just removed both parameters, thank you.

*
* @return array
*/
public function events($username, $page = 1, $perPage = 30)
Copy link
Collaborator

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

Copy link
Contributor Author

@richard015ar richard015ar Apr 21, 2021

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.

@richard015ar
Copy link
Contributor Author

@acrobat thank you for reviewing and suggesting. I submitted the changes.
Regards.

Copy link
Collaborator

@acrobat acrobat left a 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:

/**
* @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!

@richard015ar
Copy link
Contributor Author

@acrobat yes sorry I did it yesterday but I didn't added the file.
Regards.

@acrobat acrobat merged commit 54ddd41 into KnpLabs:master Apr 23, 2021
@acrobat
Copy link
Collaborator

acrobat commented Apr 23, 2021

No problem @richard015ar, thanks! And congrats on your first contribution! 🎉

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