Skip to content
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

Allow getting repo info by id, not just user and repo name #579

Merged
merged 5 commits into from
May 8, 2017

Conversation

violuke
Copy link
Contributor

@violuke violuke commented May 6, 2017

This is an undocumented feature, but GitHub support have said it can be relied on. It's also really handy!

GitHub support said:

Thanks for reaching out. Indeed that is undocumented, but that's intentional for now. We'll be documenting that endpoint in the future (actually, it's not just that endpoint, you can replace a name with the ID for most other endpoints), but I can't promise when it will happen. Still, you can rely on this behavior in your scripts and applications -- we don't have any plans on changing it.

See piotrmurach/github#283 and piotrmurach/github#282

/**
* Get extended information about a repository by its id.
*
* @link http://developer.github.com/v3/repos/
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a note saying it isn't documented...

@m1guelpf
Copy link
Contributor

m1guelpf commented May 6, 2017

@violuke This is awesome! I normally have to parse repo full_name to use the API...
Anyway, could you please add tests?

@violuke
Copy link
Contributor Author

violuke commented May 6, 2017

@m1guelpf Yes, added test and good thinking to note that it's undocumented in the code, that's done!

doc/repos.md Outdated
@@ -48,6 +48,12 @@ $repos = $client->api('repo')->find('chess', array('language' => 'php', 'start_p
$repo = $client->api('repo')->show('KnpLabs', 'php-github-api')
```

or

```php
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add the note about the undocumented feature also here in the docs. This makes it more visible for users using this api method. We can remove the note later when it's official mentioned in the docs!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, done too

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, I really like this PR. Just had a minor comment.

*/
public function showById($id)
{
return $this->get('/repositories/'.rawurlencode($id));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't the ID always an integer? Why do we ned rawurlencode?

´return $this->get(sprintf('/repositories/%d', $id));`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it should always be an integer (currently anyway). I thought about checking that they'd input an integer and if not throwing an exception, but then figured that it was better than us assuming IDs never become non-integers to just pass it over to GitHub and let them respond accordingly. Having the rawurlencode ensures that whatever is passed into the function (even if it shouldn't have been) is sent over to GitHub properly.

There probably isn't much in it as I think it's highly unlikely that GH will ever start to issue non-integer IDs, but personally I prefer this way - it's GitHub's ID, let's let them validate it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, okey. I see that @m1guelpf and @acrobat are fine with the way it is so let's not change it.

@Nyholm Nyholm merged commit b3b4db4 into KnpLabs:master May 8, 2017
@Nyholm
Copy link
Collaborator

Nyholm commented May 8, 2017

Thank you for this feature.

@m1guelpf
Copy link
Contributor

m1guelpf commented May 8, 2017

@Nyholm Do you follow semver?

@Nyholm
Copy link
Collaborator

Nyholm commented May 8, 2017

Of course. Why?

@m1guelpf
Copy link
Contributor

m1guelpf commented May 8, 2017

@Nyholm Could you release a patch version with this functionality?

@acrobat
Copy link
Collaborator

acrobat commented May 8, 2017

I guess it will be a minor release as this is a new api method?

@Nyholm
Copy link
Collaborator

Nyholm commented May 8, 2017

No we cant. This is a new feature. That means that we have todo a minor release.

I added a milestone for the next release. See https://github.com/KnpLabs/php-github-api/milestone/4.

@violuke violuke deleted the patch-1 branch May 18, 2017 16:47
@genintho genintho mentioned this pull request Jul 3, 2020
acrobat pushed a commit that referenced this pull request Jul 4, 2020
This PR was squashed before being merged into the 2.x branch.

Discussion
----------

Revamp of #372 which was abandonned, and following the example of #579


As of 2020, gettting data by ID is still undocumented. I contacted Github support to make sure it could be relied on.


> <img width="691" alt="Screen Shot 2020-07-02 at 10 12 15 PM" src="https://user-images.githubusercontent.com/664857/86434021-2d17f700-bcb1-11ea-9f19-2008ce47412d.png">


------
I have been working with an old application and I have to deal with actions made by user that have changed their login since. The old login are now used by totally different people, which can be problematic.

Commits
-------

5ed46cc Update User.php
5703492 Add unit test
62b43ab Documentation
cdaad05 Remove usage of rawurlencode
@factoidforrest
Copy link

Is this still undocumented?

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.

5 participants