Skip to content

add the ability to fetch repositories for a specific installation and user #578

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 1 commit into from
May 8, 2017
Merged

add the ability to fetch repositories for a specific installation and user #578

merged 1 commit into from
May 8, 2017

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