Skip to content

Add RepositoriesService method GetByID for fetching a repository by its numeric ID#60

Closed
sqs wants to merge 1 commit into
google:masterfrom
sqs:Repositories_GetByID
Closed

Add RepositoriesService method GetByID for fetching a repository by its numeric ID#60
sqs wants to merge 1 commit into
google:masterfrom
sqs:Repositories_GetByID

Conversation

@sqs
Copy link
Copy Markdown
Contributor

@sqs sqs commented Oct 3, 2013

This PR adds a GetByID(id int) method to RepositoriesService that fetches a GitHub repository by its numeric ID from the undocumented /repositories/:id endpoint.

Being able to fetch a repository by its ID is useful for tracking it when its name changes. The API does not redirect from the old name to the new name (and even if it did, if you created a 2nd repo with the old name, the redirect would have to be removed). The numeric ID is also useful because it takes up less space than the (owner, name) tuple.

Users should use this method with caution ⚠️ because it uses an undocumented API endpoint. @willnorris, if you think it's a bad idea to merge this method in, I would understand.

@willnorris
Copy link
Copy Markdown
Collaborator

interesting, I had no idea this existed... kinda cool. I've certainly run into the renaming issue as well. It looks like at least some of the other repo-related endpoints have equivalents using this style. For example:

My only hesitation with adding GetByID is whether that sets precedent for adding all these others as well. Maybe that's not such a bad thing? I don't know. Thoughts?

@technoweenie
Copy link
Copy Markdown

You can rely on it. In fact, we encourage it, since repository names can change. We actually rewrite endpoints like "repos/owner/name/" to "repositories/123/".

Edit: I should add, there's a similar endpoint for users: "/user/:id".

@sqs
Copy link
Copy Markdown
Contributor Author

sqs commented Oct 3, 2013

Thanks, @technoweenie!

@willnorris Yeah, it would essentially double the number of methods to support both owner/repo and ID schemes, which would not be ideal.

I suppose there could be only one method to handle both cases that would take a parameter that has a concrete value of either RepositoryByName{"owner", "repo"} or RepositoryID{123}, both of which implement some RepositorySpecifier interface. (And same for users.) This would be a backwards-incompatible change, of course.

If we were starting from scratch, I would probably want this latter solution.

@willnorris
Copy link
Copy Markdown
Collaborator

@sqs Aside from Google, I suspect that SourceGraph is one of the larger users of the library. If you're fine with the change, I don't mind either at this stage.

Would you mind taking a first pass at supporting both name and ID in a single function? The thing to keep in mind is that URL construction will need to change as well, since it's different for different identifiers... repos/{owner}/{repo} vs repositories/{id}. Just start with one function while we work out the best syntax.

@sqs
Copy link
Copy Markdown
Contributor Author

sqs commented Oct 15, 2013

Yep, will do.

@sqs
Copy link
Copy Markdown
Contributor Author

sqs commented Oct 22, 2013

FWIW, just noting it here in case anyone else experiences a similar issue, I noticed that there are some cases where the "owner/repo" endpoint succeeds and the numeric ID repositories endpoint fails (intermittently). E.g., right now, curl -v 'https://api.github.com/repos/rubyzip/rubyzip?client_id=xxx&client_secret=yyy' succeeds reliably and curl -v 'https://api.github.com/repositories/935822?client_id=xxx&client_secret=yyy' fails reliably. This may be due to caching (the "owner/repo" endpoint is hit more frequently and the response I wanted may have been cached) and has only occurred when GitHub.com is also acting up (while getting to this page where I'm typing this issue, I encountered 3 error pages and status.github.com says they are investigating "performance problems").

So far we haven't noticed a big difference in reliability, but now that we're aware of the potential for different behavior, we'll be on the lookout.

@sqs
Copy link
Copy Markdown
Contributor Author

sqs commented Nov 7, 2013

Closing and moving discussion to the more general potential solution at #68.

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.

3 participants