-
-
Notifications
You must be signed in to change notification settings - Fork 598
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
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.
Minor comments
lib/Github/Api/User.php
Outdated
* | ||
* @return array list of followed users | ||
*/ | ||
public function following($username) | ||
public function following($username, array $parameters = array(), array $requestHeaders = array()) |
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.
Use the short array syntax instead of array()
lib/Github/Api/User.php
Outdated
* | ||
* @return array list of following users | ||
*/ | ||
public function followers($username) | ||
public function followers($username, array $parameters = array(), array $requestHeaders = array()) |
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.
Use the short array syntax instead of 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. |
I threw in the change you requested for this pull request |
Can you do it also for the second method? After that the pr is good to go! |
done and done. |
No I meant |
sorry. I got confused. I reverted the change to the abstract and modified the line you were talking about. |
Is this PR ready to go? |
Yes, sorry for the delay! Thanks @miscbits |
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.