[WIP] Repository Contents Client

[khellang]

Figured I might as well open this PR to get some feedback right away 😄

[khellang]

I moved GetReadme and GetReadmeHtml to the new IRepositoryContentsClient to reflect the API better and obsoleted the old methods. Hope that's OK 😄

[khellang]

Also, if you want me to strip the first commit (changed RepositoryComments to Comments and PullRequest to PullRequests), please shout out.

[shiftkey]

I moved GetReadme and GetReadmeHtml to the new IRepositoryContentsClient to reflect the API better and obsoleted the old methods.

👍

Also, if you want me to strip the first commit (changed RepositoryComments to Comments and PullRequest to PullRequests), please shout out.

👍

[shiftkey]

I do like this approach as we get towards 1.0 and better structure the clients. Thanks!

[khellang]

@shiftkey Does that 👍 mean that you want me to strip the commit? 😝

[shiftkey]

@shiftkey Does that 👍 mean that you want me to strip the commit? 😝

My bad, pre-coffee me can be unclear. No, that's fine as-is.

[khellang]

Uuuh, I just saw #231. It hasn't been updated in 3 months. Should I still follow through with this or wait for the other PR to land?

[shiftkey]

@khellang 👍 to keep going

[khellang]

To get the actual content, how do you feel about having a single method Task<IReadOnlyList<DirectoryContent>> GetForPath(string owner, string name, string path) which gets the parent directory and checks if the requested content is a file, dir, symlink or submodule. It then returns either a list with a single file, symlink or submodule, or a directory listing. We could then provide extension methods that has relevant return values (and other validation?), e.g. FileContent, SymlinkContent or SubmoduleContent. This means that you don't have to know what type the content is before doing the call, but you have to cast (OfType<T>) afterwards. I pushed a commit to show what I mean. It's basically what @jasonrudolph mentioned in #231.

[khellang]

Actually, the more I think about it, the more I'm leaning towards differentiating GetFile, GetSymlink, GetContents, etc. and just throw if you mess up. Most clients would start out at the root anyway, right? So it would be easy to switch on the type when enumerating from there. In that way you don't get the overhead of fetching the parent directory and checking the type up front...

[khellang]

Hey guys! Sorry I haven't been able to pull this through. I might have some cycles this weekend to take a look at it again.

@shiftkey Do we really want to do it this way? E.g. get parent folder and check whether it's a file/directory/symlink etc. or do we just want to make the user check or know up front?

[shiftkey]

@khellang let's do the simplest thing that could possibly work and iterate from there

• implement a Get which takes a path and returns a file
• if the response is a collection (how to check this depends on the implementation), Get should throw to indicate the path is not a file
• implement a GetForDirectory which takes a path and returns a list of items
• if the response is an item, GetForDirectory should throw an exception to indicate the path is a file
• other bad things happen due to misusing the API, just bubble them up to the user

How does that feel?

[khellang]

@shiftkey Yes! Let's take the pragmatic approach 😄 It felt a bit wrong to do 2 API calls for each requested file/directory.

[shiftkey]

if the response is an item, GetForDirectory should throw an exception to indicate the path is a file

One note on this, you can tell it's an item based on the content property:

{
  "type": "file",
  "encoding": "base64",
  "size": 5362,
  "name": "README.md",
  "path": "README.md",
  "content": "encoded content ...",
  "sha": "3d21ec53a331a6f037a91c368710b99387d012c1",
  "url": "https://api.github.com/repos/pengwynn/octokit/contents/README.md",
  "git_url": "https://api.github.com/repos/pengwynn/octokit/git/blobs/3d21ec53a331a6f037a91c368710b99387d012c1",
  "html_url": "https://github.com/pengwynn/octokit/blob/master/README.md",
  "_links": {
    "git": "https://api.github.com/repos/pengwynn/octokit/git/blobs/3d21ec53a331a6f037a91c368710b99387d012c1",
    "self": "https://api.github.com/repos/pengwynn/octokit/contents/README.md",
    "html": "https://github.com/pengwynn/octokit/blob/master/README.md"
  }
}

[thedillonb]

Is this still being worked on?

[shiftkey]

@thedillonb signs point to no.

I think it has a way to go anyway, because we were still discussing how the API should look even then...

[drusellers]

👍

[drusellers]

What's the current blocker? Is there a punch list of what needs doing?

[shiftkey]

@drusellers I'm going to see if I can spend the next couple of weeks on clearing out the backlog of PRs - see the next major release here https://github.com/octokit/octokit.net/milestones/v0.6%20-%20Chocolate-Covered%20Yaks

This one didn't make the cut because I'm still not sold on the API. I need to think on this some more before I can work out a way through it. Let me know if this is something that you're interested in contributing to, and I'll bump it up my priority list to revisit.

[khellang]

I can also punch the code if we nail the desired API down 😄

[shiftkey]

@khellang will you be at Summit? If so, that'll give me a hard deadline to put pen to paper... 😛

[khellang]

@shiftkey Yep 😄 Looking forward to the spec. then 😉

[Woland2k]

Just in case someone really wants to use new Contents API before the official merge is done, I created an implementation for delete/create/update APIs. It is very raw, but at least you don't have to it yourself. Here is a fork: https://github.com/VirtoCommerce/octokit.net

[haacked]

Just an update. i'm looking into this right now and will push a new PR that incorporates these PRs.

[khellang]

👍

[Woland2k]

Great, can't wait to see the updates!

[haacked]

Ok, I'm going to take this in a slightly different direction. Rather than have different types for Submodule, File, Directory, I'm just going to have a single type that has the union of all the properties. The properties not in use will simply be null in the response. This ends up keeping things really simple. I'll post a PR soon for comment.

[drusellers]

👍

[haacked]

Closing in favor of #649