Skip to content

Add `post_form' for async#236

Closed
objmagic wants to merge 1 commit into
mirage:masterfrom
objmagic:master
Closed

Add `post_form' for async#236
objmagic wants to merge 1 commit into
mirage:masterfrom
objmagic:master

Conversation

@objmagic
Copy link
Copy Markdown
Contributor

Resolves #235

@rgrinberg
Copy link
Copy Markdown
Member

  1. The original function is very strange to me. Why would ~params be typed as a header? It should simply be a key-value list. Nevermind that Header.t coincides with that now. @avsm ?

  2. I don't like the copy pasting between lwt/async. Can we simply have a backend independent Request.url_encoded that will construct the request for lwt/async?

@objmagic
Copy link
Copy Markdown
Contributor Author

Yeah, simple copy pasting is so easy to do. I think cohttp needs to find a better way to solve those inconsistencies.

@rgrinberg
Copy link
Copy Markdown
Member

In this particular case pulling out the request construction to Request works good. Still doesn't address 1 however.

@avsm
Copy link
Copy Markdown
Member

avsm commented Jan 26, 2015

iirc, the form handling is vestigial from the original ocaml-http library. I see no reason why it shouldn't be built over the core HTTP library, as it's just a client that adds specific headers and a body format.

@rgrinberg
Copy link
Copy Markdown
Member

@avsm and why is ~params of type Header.t? That seems wrong to me.

@avsm
Copy link
Copy Markdown
Member

avsm commented Feb 5, 2015

@rgrinberg I agree that ~params being a Header.t seems wrong (although it's almost the same data type, with a slightly different encoding, so it's not fully weird, just named wrongly).

@rgrinberg
Copy link
Copy Markdown
Member

@marklrh Can you have a look #257 and make the same change in this PR. Should be good to merge after that.

I've tried to think of a way to get rid of the code duplication but I don't want Request to depend on Body.

@objmagic
Copy link
Copy Markdown
Contributor Author

objmagic commented Feb 8, 2015

since i deleted the repo days ago, I have to create a new pull at #258

@objmagic objmagic closed this Feb 8, 2015
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

Successfully merging this pull request may close these issues.

No post_form for async

3 participants