CURL review request

[jdrewsen]

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