Skip to content

added request params and headers to user followers calls #648

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

Merged
merged 5 commits into from
Dec 8, 2017
Merged

added request params and headers to user followers calls #648

merged 5 commits into from
Dec 8, 2017

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