Curl wrapper

[Andrei Alexandrescu]

On 05/16/2011 04:07 PM, jdrewsen wrote:
> Hi,
>
> I've been working on a wrapper for the etc.c.curl module. It is now
> pretty stable and I would very much like some feedback on the API.
>
> http://freeze.steamwinter.com/D/web/phobos/etc_curl.html
>
> BTW I use template mixins which doesn't seem to get included in the
> generated docs. Is there any way I can make this work?
>
> /Jonas

Now that's one sight for sore eyes. Nice work. A few comments and nits:

1. Http can't be a class. Network resources are the poster child of 
expensive resources that are NOT memory and don't work well with garbage 
collection. RAII is the poster child of deterministic resource 
management. So...

2. Furthermore I see customization via delegates (onReceive etc). That 
should be a good proxy for customization instead of using inheritance.

3. Data on wire is ubyte[], not void[].

4. Shouldn't onSend take a ref to its argument in case it wants to 
expand it?

5. Why HttpMethod and not Http.Method?

6. "Do not use the same instance of this class in two threads 
simultanously." -> remove. A D program isn't supposed to do that unless 
you offer shared methods.

7. HttpStatusLine -> Http.StatusLine. One up for encapsulation.

8. HttpMethod -> Http.Method. Ditto.

9. "this(in const(char)[] url);" -> "this(string url);" I assume you 
store a copy of the URL inside by calling e.g. to!string. If the client 
has a string, you refuse to tap into that nice information and share the 
string safely and efficiently, and make a copy of it for no good reason. 
If the client doesn't have a string, no worry, the compiler will tell 
her she needs to do a copy.

10. addHeader etc. Same comment. Use string whenever you want to keep a 
copy anyway.

11. setTimeCondition -> use core.Duration for time representation 
throughout.

12. AsyncHttpResult -> Http.AsyncResult :o)

13. HttpResult -> Http.Result

14. Isn't the max redirect configurable via a parameter?

15. Typo: "Callbacks is not supported..."

16. Documentation should point to descriptions of the HTTP methods 
wrapped (e.g. "post", "del" etc).

17. There should be an example with login/password.

18. See onReceiveHeader">std.curl.Curl.onReceiveHeader and others look 
like doc macros gone wrong.

19. "chuncked" -> "chunked"

20. "Max allowed redirs. -1 for infinite." -> use uint and uint.max for 
infinite.

21. "short isRunning()" -> what's the semantics of the short?

22. byLine/byChunk should not expose a string or generally data that can 
be shared safely by the client. That's because it would need to create a 
new string with each iteration, which is very inefficient. Instead, they 
should expose char[]/ubyte[] just like File.byLine does, and reuse the 
buffer with each call. Essentially byLine/byChunk should be 
near-zero-overhead means to transfer and inspect arbitrarily large streams.

23. Ftp should have get in-memory and streaming byLine/byChunk, not only 
get to file.

24. The shutdown mechanism should be handled properly. Shutting down 
libcurl would have all pending transfers instantly throw a special 
exception. Without a shutdown API, applications won't be able to 
implement e.g. a Quit button.

Again, great work!


Andrei