Use Yojson to parse github json responses

[gpetiot]

First step towards fixing #175

[NathanReb]

This is a much welcome change! Thanks for working on this.

We could go even further but this is already a very good first step!

Some things need to be adjusted though. I think every function parsing an API response should be tested and in particular it should behave properly on the example response from https://developer.github.com/v3.
These kind of test would rule out the bug with Location for instance!

[NathanReb]

Changes look good! I have a few suggestions to slightly improve it, let me know what you think!

[gpetiot]

@NathanReb I have a weird bug when testing this PR with dune-release-testing (when uploading the release asset), and I'm at the point where I'm doubting my testing setup is messed up (I reduced this PR to the minimum), can you confirm you get a similar error?

[NathanReb]

I just went through releasing dune-release-testing from master where it worked well and with your branch where I indeed got an error:

$ dune-release publish
[-] Publishing documentation
[-] Selected packages: dummy
[-] Generating documentation from _build/dummy-v0.2.0+test.tbz
[-] Publishing to github
[-] Updating local gh-pages branch
[+] No documentation changes for dummy 0.2.0+test in directory . of gh-pages branch
[-] Publishing distribution
[-] Publishing to github
[?] Push tag v0.2.0+test to git@github.com:NathanReb/dune-release-testing.git? [Y/n]

[-] Pushing tag v0.2.0+test to git@github.com:NathanReb/dune-release-testing.git
[?] Create release v0.2.0+test on git@github.com:NathanReb/dune-release-testing.git? [Y/n]

[-] Creating release v0.2.0+test on git@github.com:NathanReb/dune-release-testing.git via github's API
[+] Succesfully created release with id 25850938
[?] Upload _build/dummy-v0.2.0+test.tbz as release asset? [Y/n]

[-] Uploading _build/dummy-v0.2.0+test.tbz as a release asset for v0.2.0+test via github's API
dune-release: [ERROR] Line 1, bytes 0-33:
                      Invalid token 'HTTP/1.1 201 Created
                      Cache-Contr'

[gpetiot]

I found what the probllem is, in some cases Curly doesn't correctly split the headers from the body, so everything ends up in the body, that cannot be parsed as a json entity. I defined a redundant function in Curl.Response.process to fix that.
I plan to open an issue for Curly but as we don't know when it will be fixed I choose to implement the ugly workaround anyway.

dune-release correctly works with this patch.

[gpetiot]

It turns out the issue was due to a missing -D argument to dump the headers, otherwise they end up in the body of the response. I think it's ready for a final review now.

edit: I will let the tests as is afterall, @NathanReb can we merge it?

[pitag-ha]

This seems like a very nice change to me. Before, for me, it was pretty tedious and hard to read through that part of the code and now it has become far clearer and also seems less error-prone. Thanks!

Of course, I'm missing a lot of context by the long history of discussion you've had on this, so there might be things I'm missing. But the code seems clear.

[pitag-ha]

For me it has been a bit hard to understand why the "-D -" is necessary even after reading the commit message. I've understood it after reading the discussion. I'm wondering if it might be worth improving the commit message (or even leaving a comment here); for example, by mentioning that having the headers in the body messes up the JSON-structure of the body and by choosing a sentence that makes clear the causality (no "-D" argument => headers in the body). The latter e.g. by replacing "missing" with "added" in your commit message or so. What do you think?

[gpetiot]

That's true this is a bit obscure. I'm not sure the commit message is a good place for this, I was thinking of adding a few comments in the code to explain the arguments or define a simple abstraction to have a named variable for each argument, instead of handling string literals (curly should have done that, since it's only curl arguments but it can't hurt doing it ourselves) but this is a bigger change so it would be in another PR.

[pitag-ha]

I like the idea of making that precise inside the code either by named variables or by comments. So yes, let's leave that for another PR.

[NathanReb]

Looks great, thanks!