Skip to content
This repository has been archived by the owner on Jun 20, 2022. It is now read-only.

[WIP] Add markdown API #112

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

[WIP] Add markdown API #112

wants to merge 5 commits into from

Conversation

obsc
Copy link
Contributor

@obsc obsc commented Apr 30, 2015

Added functionality for https://developer.github.com/v3/markdown/

@pengwynn It seems like the markdown API is one of the few (only?) API endpoints that return html rather than json. However, there is no support for encoding and decoding text/html;charset=utf-8. This is due to the fact that go-sawyer was only initialized with an encoder and decoder for json: https://github.com/jingweno/go-sawyer/blob/master/sawyer.go#L82-L89

At the moment, the workaround I implemented is kind of hacky: I get the raw response back from the server and then directly parse it into a string.

Additionally, encoding a string with setBody defaults to using json encoding as well. This results in strings getting encoded with two extra quotation marks:
testBody(t, r, "\"Hello world github/linguist#1 **cool**, and #1!\"\n")
should be
testBody(t, r, "Hello world github/linguist#1 **cool**, and #1!\n")

What are your thoughts about this?

Edit: Just realized I accidentally used windows line breaks

@feiranchen
Copy link
Contributor

I don't have better ideas for html parsing either, cuz it doesn't seem to fall in the scope of sawyer, nor is there common use cases.

However, it seems like you are just checking for raw text, maybe a shorter sample text will just do for testing purposes? Especially since we are testing on client side only?

Just my 2 cents, feel free to comment!

@obsc
Copy link
Contributor Author

obsc commented May 1, 2015

Hmm, a shorter sample text could make the tests simpler. But the main issue lies with decoding the response. We don't necessarily need html parsing per se, but might need a better way to parse raw text from the response. Right now, I have to use ioutil to transform the body of the response into a bitarray to work with.

@pengwynn
Copy link
Collaborator

pengwynn commented May 3, 2015

It seems like the markdown API is one of the few (only?) API endpoints that return html rather than json. However, there is no support for encoding and decoding text/html;charset=utf-8. This is due to the fact that go-sawyer was only initialized with an encoder and decoder for json:

This method is indeed an outlier. We have other cases for returning non-JSON, but I don't think we have anywhere where we're POST-ing non-JSON.

Have you tried sending an Accept header?

@feiranchen
Copy link
Contributor

Oops, I pushed before I saw your comment.
However I just tried and an Accept header didn't help.
Also, I looked into getBody and it can't be used off-the-shelf mainly because result.Response.Body is a bit stream that needs to be decoded. That means using it doesn't make Rene's version any cleaner, I believe.

I've tried to address the problems Rene brought up. In the commit above, I did the following:

  • coded an encoder for raw text request and the two extra quotation marks went away.
  • re-factored the "decoder" for raw response into method createResponseRaw. However there probably exists a clear approach.
  • expanded Render into methods Render and RenderRaw for clarity

Please take a look at this version and let me know what you think!

return "", &Result{Err: err}
}
result = sendRequest(m.client, url, func(req *Request) (*Response, error) {
req.Header.Set("Accept", textMediaType)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@feiranchen Are you sure accepting a textMediaType works here? If you actually test by sending a POST request to this endpoint with an ACCEPT of text/plain;charset=utf-8, it will respond with an error 415. Trying an ACCEPT of application/json;charset=utf-8 or application/vnd.github.v3.html;charset=utf-8 works fine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll delete this line then. It wasn't doing anything haha. Thanks for testing!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants