Support both named and numeric ID schemes

[sqs]

For context: #60

This PR is a prototype for supporting both named (owner/repo) and numeric ID schemes. It only includes modifications to 3 repository methods for now, as we figure out the right way to implement this.

[willnorris]

👍 I like it! I really don't think I'd change a thing.

[sqs]

Great! I'll update this PR with the same change applied to the other methods.

[sqs]

Sorry--I dropped the ball on this. Now that the library has grown in popularity, is a breaking change of this magnitude still possible?

[willnorris]

the problem is, it's very hard to know how popular the library really is and what the impact of such a change would be. I did notice recently (maybe as part of all the API changes that went live this week?) that pagination links now use ID based URLs...

% curl -s -I "https://api.github.com/repos/google/go-github/stargazers?per_page=1" | grep ^Link
Link: <https://api.github.com/repositories/10270722/stargazers?per_page=1&page=2>; rel="next", <https://api.github.com/repositories/10270722/stargazers?per_page=1&page=393>; rel="last"

That certainly makes me want to move forward with this change, though that still doesn't mean it's a great idea 😕

[sqs]

Interesting re: the ID-based URLs in the new pagination links.

Incidentally, there is a sketchy backwards-compatible way of supporting both named and numeric ID schemes:

• For repositories, if the owner is "", then treat the repo name argument as the string representation of the numeric repo ID.
• For users, if the user login argument starts with "#", then treat the rest of the login as the string representation of the numeric user ID. (The special prefix is necessary for disambiguation; e.g., I am GitHub user ID 1976 but there is also a user named @1976.)

Anyway, if you decide you're OK moving forward with this change (of the kind I prototyped in this PR, not the sketchy backcompat one above), I will actually submit a PR promptly (in a couple of weeks). As for determining the impact of the change, we could reach out to some of the folks who have projects that use go-github, but that is definitely an incomplete list since most people probably use it in internal apps. I am happy to help reach out to gauge the impact; just let me know.

[willnorris]

sigh, I'm embarrassed to admit that it didn't occur to me to use souregraph to determine who all is using the library 😳

Given that there aren't that many people using it, and the ones using it for in-house apps likely (hopefully?) have a stable snapshot, I say let's go for it. Plus, we're really close to 100% API coverage (we actually were at 100% briefly last week, until I found a bunch of newer endpoints I had missed). With that plus this final major change, we can mostly stick a fork in things until there are changes on GitHub's side. I still want to add more integration tests, but that doesn't effect the library stability at all.

So yeah, let's go for it.

[sqs]

Awesome. 🚀 I hope to have this mostly wrapped up in a couple of weeks and will push progress here as I make it.

[willnorris]

One additional thought I had last week...

type RepositorySpec interface {
    Path() string
}

All of our *Spec interfaces are going to end up being identical, no? In which case, there's probably no point in defining multiple. We could have a single interface like ResourceSpec or whatever, but then we lose type safety... there's nothing preventing someone from passing a RepositoryID to the GetUser() method. It would still fail, but at runtime rather than compile time. We could make each *Spec interface unique by renaming Path() to something like Repo() string. Thoughts?

[sqs]

Yes, great point. I think your proposed solution is great.

[phuna]

Hi, I'm getting into the same issue when a repository is renamed then github API will return 404 for it if using named URL. Is there any plan to resolve conflict & merge this PR yet?

Thank you

[willnorris]

closing this, since we're long past the ability to make this change and @shurcooL just added new methods to handle ID based fetching in #329

[dmitshur]

I have to admit, I did not see the actual content of this PR until just now. I mistakenly thought this was the same change as #60.

Now that I've seen it, I completely agree with #68 (comment). I like it a lot too.

Both luckily and unfortunately, it is too late to much such a change now, so nothing would've changed even if I had seen it. But I'll definitely remember this idea for reference if I'm ever designing API/client libraries in the future.