Skip to content

Conversation

@miscbits
Copy link
Contributor

This change is a non-breaking way to be able to add parameters and request headers to these two calls. In my honest opinion, this change should be applied to all calls since it allows users an easy way to set custom parameters that GitHub may add even if they don't get implemented in this package immediately. This change also does not affect users who simply use the setPerPage method so it is simply an alternate way to page as well.

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.

Minor comments

* @return array list of followed users
*/
public function following($username)
public function following($username, array $parameters = array(), array $requestHeaders = array())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use the short array syntax instead of array()

* @return array list of following users
*/
public function followers($username)
public function followers($username, array $parameters = array(), array $requestHeaders = array())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use the short array syntax instead of array()

@miscbits
Copy link
Contributor Author

protected function get($path, array $parameters = array(), array $requestHeaders = array())

@acrobat I did it this way because it matches this signature. If I change it to use shorthand array here, it should also be changed in AbstractApi.

@miscbits
Copy link
Contributor Author

I threw in the change you requested for this pull request

@acrobat
Copy link
Collaborator

acrobat commented Nov 27, 2017

Can you do it also for the second method? After that the pr is good to go!

@miscbits
Copy link
Contributor Author

done and done.

@acrobat
Copy link
Collaborator

acrobat commented Nov 27, 2017

No I meant lib/Github/Api/User.php line 112. Leave the abstract class for now. Thanks!

@miscbits
Copy link
Contributor Author

sorry. I got confused. I reverted the change to the abstract and modified the line you were talking about.

@miscbits
Copy link
Contributor Author

miscbits commented Dec 8, 2017

Is this PR ready to go?

@acrobat
Copy link
Collaborator

acrobat commented Dec 8, 2017

Yes, sorry for the delay! Thanks @miscbits

@acrobat acrobat merged commit 9bc5675 into KnpLabs:master Dec 8, 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