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

How to handle files in a gist response #215

Closed
SimonCropp opened this issue Nov 13, 2013 · 18 comments
Closed

How to handle files in a gist response #215

SimonCropp opened this issue Nov 13, 2013 · 18 comments

Comments

@SimonCropp
Copy link
Contributor

From here
http://developer.github.com/v3/gists/#get-a-single-gist

the response will be

{
  "url": "https://api.github.com/gists/5a7e1513d4ad4ef62674",
  "id": "1",
  "description": "description of gist",
  "public": true,
  "user": {
    "login": "octocat",
    "id": 1,
    "avatar_url": "https://github.com/images/error/octocat_happy.gif",
    "gravatar_id": "somehexcode",
    "url": "https://api.github.com/users/octocat"
  },
  "files": {
    "ring.erl": {
      "size": 932,
      "filename": "ring.erl",
      "raw_url": "https://gist.github.com/raw/365370/8c4d2d43d178df44f4c03a7f2ac0ff512853564e/ring.erl"
    }
  },

Note that each file node is named based on the name of the file. In this case the name is ring.erl.

How would this be handled by the octokit deserializer?

@SimonCropp
Copy link
Contributor Author

related to #216

@haacked
Copy link
Contributor

haacked commented Nov 13, 2013

I'm going to bet we don't handle that at all, but I haven't tried it. We might need to customize this case. Either "files" is a dynamic object OR files deserializes into a dictionary. The dictionary approach is my preference.

@haacked
Copy link
Contributor

haacked commented Nov 13, 2013

If "files" is a Dictionary<File> I wonder if this just works.

@SimonCropp
Copy link
Contributor Author

I may have time to look into this on the weekend.

@SimonCropp
Copy link
Contributor Author

@haacked but while we are here what is the value in returning a response like that? would not a simple list be better?

@shiftkey
Copy link
Member

@SimonCropp it would be better but for the sake of not overcomplicating the deserializing hooks (or breaking things completely) you can just enumerate the dictionary:

public class Gist 
{
  public IDictionary<string, File> Files { get; set; }
}

// elsewhere

foreach(var file in gist.Files)
{
  // do stuff
}

@SimonCropp
Copy link
Contributor Author

@shiftkey r u sure you would not prefer

public IList<File> Files { get; set; }

and handle the fact that it is a dictionary internally?

@haacked
Copy link
Contributor

haacked commented Nov 13, 2013

I guess it depends on whether it's useful to look up files by name. If not, then an IReadOnlyList<File> makes sense.

@SimonCropp
Copy link
Contributor Author

well u can still look up by name since the name is also on the file. would just need a linq statement instead of a dictionary lookup. I think in this case there will be little perf difference. unless people have 1000s of files a gist... is this likely?

@haacked
Copy link
Contributor

haacked commented Nov 13, 2013

Not likely so i agree with the list. :)

@SimonCropp
Copy link
Contributor Author

collaboration FTW :)

@SimonCropp
Copy link
Contributor Author

BTW is there any other place where you guys are doing custom deserialization so i can follow the same approach/conventions?

@haacked
Copy link
Contributor

haacked commented Nov 14, 2013

@SimonCropp
Copy link
Contributor Author

ok so i noticed you guys already support dictionary. so i am being lazy and leaving it as that for now :)

before i do a PR i have a couple of questions.

  1. Do u want a PR to master?
  2. Should I be reusing the common User for Gist.User and GistFork.User?
  3. In my test Gist.User returns null. Is this because i am doing point 2 wrong?

https://github.com/SimonCropp/octokit.net/commit/0b0661aa1e74aa84bed2c21d3a2abf1d0b3c7713

@haacked
Copy link
Contributor

haacked commented Nov 14, 2013

  1. Yes
  2. Yes.
  3. Not sure.

On Wed, Nov 13, 2013 at 5:48 PM, Simon Cropp notifications@github.comwrote:

ok so i noticed you guys already support dictionary. so i am being lazy
and leaving it as that for now :)

before i do a PR i have a couple of questions.

  1. Do u want a PR to master?
  2. Should I be reusing the common User for Gist.User and GistFork.User?
  3. In my test Gist.User returns null. Is this because i am doing point
    2 wrong?

SimonCropp@0b0661ahttps://github.com/SimonCropp/octokit.net/commit/0b0661aa1e74aa84bed2c21d3a2abf1d0b3c7713


Reply to this email directly or view it on GitHubhttps://github.com//issues/215#issuecomment-28453023
.

@shiftkey
Copy link
Member

Regarding question 3:

Note: When using the v3 media type the “user” field will become “owner”

I think this is now the default behaviour.

screen shot 2013-11-13 at 8 30 33 pm

@haacked
Copy link
Contributor

haacked commented Nov 14, 2013

Ah, let's make sure we're requesting the v3 media type then.

@shiftkey
Copy link
Member

Fairly certain we've sorted this with #225. Closing.

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

No branches or pull requests

3 participants