Den 24-08-2011 13:04, Johannes Pfau skrev:
dav1d wrote:
Jonas Drewsen Wrote:

Hi all,

     This is a review request for the curl wrapper. Please read the
"known issues" in the top of the source file and if possible suggest
a solution.

We also need somebody for running the review process. Anyone?

Code:
        https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d      
Docs:
        http://freeze.steamwinter.com/D/web/phobos/etc_curl.html

Demolish!

/Jonas

Hello,

At first a little story, to skip it just begin reading after the
paragraph. I'm still a D "beginner", I set myself as goal translating
an already written Python lib into D, which isn't easy with broken
"std.json" etc. so I was quite happy when I found etc.curl (#curl on
freenode linked me there). Well I was a bit surprised when I saw Http
is a struct, anyway it works and looks good. So while I was working I
encountered that the website I sent the requests to always gave me
error codes, I was wondering why, I set _all_ headers correctly
(checked it twice) (Yeah I've to set a "Cookie" Header...) and the
requests also looked correct .. so I started Wireshark sniffing what
actually is beeing sent .. I realized: not a single Header is sent,
WTF, so I took another look in the code and I went crazy ..

Setting headers to the Http struct works .. but if you call .post a
Result struct is created and just one Header "Content-Type" is passed
to it and then you .postData gets called on the Result struct. (look
at the name .. it should contain the result and not send the request).
This behavour is undocumented and totally ridiculous.

I'm coming from Python, Pythons urrlib hasn't the best api (well some
might say the api sucks) but etc.curl is on top of that.

My personal opinion is rewrite this module or dont put it in the
std.lib, if you do, this will confuse and drive lots of people crazy.

Well, your problem is caused by two things: The API docs are not very
detailed here and D allows static methods to be called on
instances, which can be confusing.

The post method is a static method, so it can't use headers you've set
with addHeader/setHeader. If you use an Http instance like this:

Http client = Http("http://google.com";);
client.addHeader("Test", "Header");

you _must not_ call client.post()
instead you have to set the method property and use perform:

client.method = Http.Method.post; //AFAIK post is default, so you can
                                   //skip this line
client.perform();

The post/get/put methods are only convenience methods with very limited
functionality. They only exist to make simple requests with one line of
code.


One way that may improve this would be to move the static methods outside the class and make them into module functions instead.

The drawbacks of this is:

1, When importing the module there would be more symbols polluting the namespace.

2, We would have to rename from Http.get to a function named HttpGet. We cannot call it simply "get" because both Http and Ftp has the get method.

And personally i lige the Http.get syntax better.

Any other suggestions?

/Jonas


Reply via email to