Skip to content

Conversation

*/
public function repositoriesByInstallation($installationId, array $params = array())
{
return $this->get("/user/installations/$installationId/repositories", array_merge(array('page' => 1), $params));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is page hardcoded?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Page is only hardcoded when you don't pass a page array key in the params array. So this is the default value

@bdelbasso
Copy link
Contributor Author

@acrobat @Nyholm does one of you has the ability to trigger the style check again? Looks like it's stuck.

@acrobat
Copy link
Collaborator

acrobat commented May 7, 2017

It looks like the styleci job for this PR was not started. You could rebase and force push which should trigger a new build

Copy link
Collaborator

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Good.

You also need to add a configure method to set Accept header to application/vnd.github.machine-man-preview+json

*/
public function repositoriesByInstallation($installationId, array $params = array())
{
return $this->get("/user/installations/$installationId/repositories", array_merge(array('page' => 1), $params));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets use sprintf here. Also, do single quotes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

@bdelbasso
Copy link
Contributor Author

@Nyholm I'm not sure about your last comment (the Accept header). I think it's outside of the scope of this PR.

If I want to call any route regarding integrations (any method in Integrations.php - which are not discussed here), I also need to set the Accept header. Which is possible in the constructor of the Client (second parameter $apiVersion).

I see two benefits of not adding it here:

  • the developer is aware that this route is early access: if the appropriate api version is not set, then he'll get an error from Github API.
  • when the route is released publicly, we don't have to change anything.

@Nyholm
Copy link
Collaborator

Nyholm commented May 8, 2017

If I want to call any route regarding integrations (any method in Integrations.php - which are not discussed here), I also need to set the Accept header. Which is possible in the constructor of the Client (second parameter $apiVersion).

I did not know. Thanks.

I see two benefits of not adding it here

I do agree with your reasons, but the configure method is needed for consistency.

I think it's outside of the scope of this PR.

Yeah, since it is needed for the existing endpoints as well. I created a new issue for that #580

@Nyholm Nyholm added this to the Release 2.4 milestone May 8, 2017
@Nyholm Nyholm merged commit a21beb3 into KnpLabs:master May 8, 2017
@Nyholm
Copy link
Collaborator

Nyholm commented May 8, 2017

Thank you for this PR.

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.

4 participants