Den 16-08-2011 21:59, Jonathan M Davis skrev:
> On Tuesday, August 16, 2011 20:23:19 jdrewsen wrote:
>> Den 16-08-2011 15:13, dsimcha skrev:
>>> On 8/16/2011 7:48 AM, 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
>>>>
>>>    From a quick look, this looks very well thought out. I'll review it
>>>
>>> more thoroughly when I have more time. A few questions/comments from a
>>> quick look at the docs:
>>>
>>> Does the async stuff use D threads, or does Curl have its own async API?
>>
>> It uses the spawn method from std.concurrency which uses D threads i think.
>>
>>> In your examples for postData, you have onReceive a ubyte[] and write it
>>> out to console. Did you mean to cast this to some kind of string?
>>
>> Not really. It is just an example and writeln will output a byte array
>> just fine.
>>
>>> For onReceive, what's the purpose of the return value?
>>
>> To specify the number of bytes that have been handled. If this is less
>> that the entire buffer the rest of the request will fail. Additionally
>> both the onSend and onReceive callbacks can return  CURL_WRITEFUNC_PAUSE
>> pr CURL_READFUNC_PAUSE to pause the transfer. And onSend can return
>> CURL_READFUNC_ABORT to abort as well. I see that the is missing from the
>> docs... will add.
>>
>>> If/when this module makes it into Phobos, are we going to start
>>> including a libcurl binary with DMD distributions so that std.curl feels
>>> truly **standard** and requires zero extra configuration?
>>
>> I think that would be convenient but don't believe it is the correct
>> thing to do. In my opinion D modules that are not pure D or wrapping a
>> system library (which libcurl is definitely not) should be the only ones
>> allowed in the std package. Other wrappers in phobos should be in the
>> etc package and the lib binaries should not be included.
>>
>> Just how I feel about it though. Anyone who disagrees?
>
> In general, external libraries should not be included in Phobos. They should
> be using whatever is on the user's system. As such, you're correct in that etc
> is probably the place to put the curl wrapper, not std.
>
> - Jonathan M Davis

It could ofcourse just be make a root module 'curl' instead of 'etc.curl'. I would like it to be etc.curl in phobos since I think D needs a set of "Phobos approved" extensions. Extensions that are guaranteed to have certain amount of quality because they have been through the review process.

Maybe at some point when a package manager of some kind is available the modules in the etc package can be torn out of the official phobos bundle and downloaded separately from d-p-l.org.

/Jonas

Den 16-08-2011 22:05, bearophile skrev:
> jdrewsen:
>
>> Other wrappers in phobos should be in the
>> etc package and the lib binaries should not be included.
>>
>> Just how I feel about it though. Anyone who disagrees?
>
> All necessary binaries of the libraries used by all Phobos modules need to be in the zip of the Windows distribution (and probably all other binary distributions of DMD too). Otherwise they are not worth adding to Phobos.
>
> Bye,
> bearophile

etc.c.curl is already in phobos and no binary library for curl is in.

I disagree on this one though. But please see my reply to jonathan as to how I see final solution.

/Jonas

Den 16-08-2011 19:15, Lars T. Kyllingstad skrev:
> On Tue, 16 Aug 2011 12:26:19 -0400, Nick Sabalausky wrote:
>
>> "David Nadlinger"<see@klickverbot.at>  wrote in message
>> news:j2e15a$2nsh$1@digitalmars.com...
>>> Seems like the reviews are already coming in steadily, but we still
>>> have no review manager - if nobody else steps up until tomorrow, I'd
>>> volunteer so we can get the formal review process going.
>>>
>>>
>> Curious: What exactly is the role of the review manager?
>
> Andrei has suggested we follow Boost's review model:
>
> http://www.boost.org/community/reviews.html
>
> -Lars

And I see that the link contains info for the library developer as well. Have to read.

/Jonas

Den 16-08-2011 19:05, Jimmy Cao skrev:
> On Tue, Aug 16, 2011 at 10:08 AM, David Nadlinger <see@klickverbot.at
> <mailto:see@klickverbot.at>> wrote:
>
>     Seems like the reviews are already coming in steadily, but we still
>     have no review manager – if nobody else steps up until tomorrow, I'd
>     volunteer so we can get the formal review process going.
>
>     David
>
>
>
>     On 8/16/11 1:48 PM, 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
>         <https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d>
>         Docs:
>         http://freeze.steamwinter.com/__D/web/phobos/etc_curl.html
>         <http://freeze.steamwinter.com/D/web/phobos/etc_curl.html>
>
>         Demolish!
>
>         /Jonas
>
>
>
>
> On line 80, I made a typo when writing the documentation:
>
> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L80
>
> It should be:
> <to.addr@gmail.com <mailto:to.addr@gmail.com>>

ok

> After reading the libcurl documentation, I realize that the automatic
> angle bracket attaching at lines 2932 and 2943 are not needed (libcurl
> seems to do it for you after version 7.21.4).

Will fix

Thanks
Jonas

jdrewsen:

> etc.c.curl is already in phobos and no binary library for curl is in.

I see. I didn't know this. Then it's the good moment to improve this situation.


> I disagree on this one though. But please see my reply to jonathan as to how I see final solution.

Most times I'll not use a Phobos module that to be used requires the compilation of a multi-file C library. I use another language where this functionality is already present, like this one:
http://docs.python.org/library/httplib.html

Accessing the web from the language today is a basic feature, and quite more commonly needed than as example complex numbers.

In Clojure even the Phobos equivalent of the File function loads from the web if you give it an URL instead  of a file path :-) Please help pushing D into this decade.

Bye,
bearophile

On 16/08/2011 17:38, Nick Sabalausky wrote:
> "Alix Pexton"<alix.DOT.pexton@gmail.DOT.com>  wrote in message
> news:j2dsq0$2aht$1@digitalmars.com...
>>
>> "...returns the number of elements in the buffer that has been filled and
>> is ready to send." ->  "...returns the number of elements in the buffer
>> that have been filled and are ready to send."
>>
>
> "in the buffer that has been filled" is correct. The "has" refers to "the
> buffer" not the elements that have been put into it. And there's only one
> buffer in question, so "has".
>
> But yea, "is ready to send" should be changed to "are ready to send".

I suspected my edit was off in this case (from a technical point of view), but it is still poor use of English (imho). If the buffer has been "filled" then the return will equal its capacity, otherwise filled seems to be the wrong word. Perhaps changed "filled" to "written"?

>
>
>> "Asynchronous HTTP _WHATEVER_ to the specified URL." ->  "Performs a HTTP
>> _WHATEVER_ on the specified URL, asynchronously."
>
> "Performs a HTTP" ->  "Performs an HTTP". The letter "H" may technically be a
> consonant, but in this case it's read as the *name* of the letter, not the
> letter's normal sound. And the name of H starts with a vowel sound (long A).
> (Besides, "a HTTP" just sounds awkward.)
>
> Also, little bikeshedding, but I think this would be better still:
>
> "Performs an asynchronous HTTP _WHATEVER_ on the specified URL."
>

I played with this word order too, but I must admit the a/an thing snook through between many of my rewrites, good catch ^^

A...

On 16/08/2011 20:44, jdrewsen wrote:
> Den 16-08-2011 15:54, Alix Pexton skrev:
>
>> Protocol.onProgress represents transfer statistics using doubles?
>
> That is what we get from libcurl

I'll let you off then, but just this once ^^

>> CurlTimeCond, members are all lower case, took me a few goes to work out
>> what they might mean, still not all that sure. Should I assume that if
>> the meanings are not obvious that I'm not ready to be tinkering with
>> curl at this level, or should each value be clarified?
>
> I will include a link to the HTTP RFC that explains it.

Seems reasonable.

>> Http.addHeader / Http.setHeader both have the same description, I assume
>> the latter overwrites an existing value if it exists? what is the
>> behaviour of addHeader when the key already exists?
>
> Added explanation and a link to HTTP RFC explaining sematics.

I read the source now, they do do as I expected, which made me wonder why there is no removeHeader, but I can't think of a reason to use removeHeader, so I won't suggest its addition ^^

>> Also, later examples are just marked as "Asynchronous version
>> of..." and lose the note about callbacks, which I assume still apply.)
>
> The latter examples are trivial HTTP methods not sending or receiving
> anything but the status codes are used. Therefore they do not need an
> elaborate example as for the GET/POST/PUT methods.

Well, my honest thought as I read through it was that you were getting fed up of writing near identical documentation for each method and had started to cut corners. Can ddoc be used to add a section in front of these functions that outlines their common features? Making it a bit DRYer?

>> All callbacks are write only properties which prevents the use of event
>> hooking (which, in my experience, is a common occurrence in event based
>> models). Hooking might not apply in this case however (I've not
>> particularly thought through any use cases) and safe event hooking might
>> be beyond the scope of this module.
>
> It is possible to provide read functionallity of the properties. The
> only issue with this is that the delegate read would not be the same as
> the one just written because it gets wrapped it the propery set call. I
> don't know if this is a problem though?

Hmn, it could well be! I've not thought of a good use case yet though, so I'd recommend leaving it as it is for now, and if D gets an Events module in the future, the hooking issue can be revisited.

>> Ftp.Result.encoding / Ftp.AsyncResult.encoding
>> I'm not sure what these functions are supposed to return, the
>> descriptions seem inadequate.
>
> I will try to improve the docs. They return the EncodingSchemes that is
> used when reading the data
>
> http://www.d-programming-language.org/phobos/std_encoding.html
>
> The scheme is changed by using the encodingName property.

I looked at the code, and for a moment I though I got it, but I was wrong ><
from the source...
> ref Result encoding(const(char)[] schemeName) {
>             rp._encodingSchemeName = schemeName;
>             return this;
>         }

I'm now guessing this is just for chaining?

>> Not fully read all the source yet, but as I'm not all that familiar with
>> curl, I'm not sure I'll spot much ^^
>>
>> Not a criticism, but at first glance, it doesn't feel very D-ey, but I'm
>> still not 100% sure what idiomatic D is yet.
>
> Maybe you're right. Whether it is a product of being a wrapper around an
> existing library and thereby forcing some design decisions or by my
> coding style I don't know.
>
> If anyone can give some input as how to make it more D'ish please do.

The source, for the most part, does feel D'ish, so you must be doing something right, but there is no point in adding extra templates and custom ranges if there is no need. I'll have a stab at writing some code with it tomorrow, to see how the bits fit together in the wild.

> Thank you the valuable feedback

No problem, I'm glad you found it so ^^

A...

On 16/08/2011 20:55, jdrewsen wrote:
> Den 16-08-2011 16:43, Alix Pexton skrev:
>> On 16/08/2011 12:48, 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
>>
>> Spotted another issue...
>>
>> In the example for Protocol.onReceive, there is a mismatch between the
>> parameter names specified for the delegate and those used inside it.
>
> you mean this one:
>
> client.onReceive = (ubyte[] data) { writeln("Got data", cast(char[])
> data); return data.length;};
>
> I cannot identify the problem?

My mistake, I was in a hurry and mixed up the functions ><
This is the snippet in question (lines 414-417 in the source)

> http.onProgress = delegate int(double dl, double dln, double ul, double ult) {
> writeln("Progress: downloaded ", dln, " of ", dl);
> writeln("Progress: uploaded ", uln, " of ", ul);
>        };

The last parameter is named "ult", but the writeln is passed "uln", I think more descriptive names might help.

A...

On Tuesday, August 16, 2011 12:32 Martin Nowak wrote:
> On Tue, 16 Aug 2011 20:48:51 +0200, jdrewsen <jdrewsen@nospam.com> wrote:
> > Den 16-08-2011 18:55, Martin Nowak skrev:
> >> On Tue, 16 Aug 2011 15:13:40 +0200, dsimcha <dsimcha@yahoo.com> wrote:
> >>> On 8/16/2011 7:48 AM, 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
> >>> 
> >>> From a quick look, this looks very well thought out. I'll review it more thoroughly when I have more time. A few questions/comments from a quick look at the docs:
> >>> 
> >>> Does the async stuff use D threads, or does Curl have its own async API?
> >>> 
> >>> In your examples for postData, you have onReceive a ubyte[] and write it out to console. Did you mean to cast this to some kind of string?
> >>> 
> >>> For onReceive, what's the purpose of the return value?
> >>> 
> >>> If/when this module makes it into Phobos, are we going to start including a libcurl binary with DMD distributions so that std.curl feels truly **standard** and requires zero extra configuration?
> >> 
> >> I was also wondering about the async handling. In the long-term I'd like
> >> to see a bigger picture for async handling in phobos (offering some kind
> >> of futures, maybe event-loops etc.).
> >> Though this is not a requirement for the curl wrapper now.
> >> std.parallelism also has some kind of this stuff and file reading would
> >> benefit from it too.
> > 
> > This has been discussed before and I also think this is very important. But before that I think some kind of package management should be prioritized (A DIP11 implementaion or a more traditional solution).
> > 
> >> One thing I spotted at a quick glance, sending to be filled buffers to another thread should not be done by casting to shared not immutable.
> > 
> > I'm not entirely sure what you mean. There is no use of shared buffers in the wrapper. I do cast the buffer between mutable/immutable because only immutable or by value data can be passed using std.concurrency. Since the buffers are only used by the thread that currently has the buffer this is safe. I've previously asked for a non-cast solution (ie. some kind of move between threads semantic for std.concurrency) but was advised that this was the way to do it.
> > 
> >> martin
> 
> Pardon the typo. What I meant is that AFAIK casting from immutable to
> mutable has undefined behavior.
> The intended method for sending a uint[] buffer to another thread is to
> cast that
> buffer to shared (cast(shared(uint[])) and casting away the shared on the
> receiving side.
> It is allowed to send shared data using std.concurrency.

Casting away immutability and then altering data is undefined. Actually casting it away is defined. So, if you have data in one thread that you know is unique, you can cast it to immutable (or std.exception.assumeUnique to do it) and then send it to another thread. On that thread, you can then cast it to mutable and alter it.

However, you're circumventing the type system when you do this. So, you have to be very careful. You're throwing away the guarantees that the compiler makes with regards to const and immutable. It _is_ guaranteed to work though. And I'm not sure that there's really any difference between casting to shared and back and casting to immutable and back. In both cases, you're circumventing the type system. The main difference would be that if you screwed up with immutable and cast away immutable on something that really was immutable rather than something that you cast to immutable just to send it to another thread, then you could a segfault when you tried to alter it, since it could be in ROM.

- Jonathan M Davis

On Tue, 16 Aug 2011 16:39:36 -0400, bearophile wrote:

> jdrewsen:
> 
>> etc.c.curl is already in phobos and no binary library for curl is in.
> 
> I see. I didn't know this. Then it's the good moment to improve this situation.
> 
> 
>> I disagree on this one though. But please see my reply to jonathan as to how I see final solution.
> 
> Most times I'll not use a Phobos module that to be used requires the compilation of a multi-file C library. I use another language where this functionality is already present, like this one: http://docs.python.org/library/httplib.html
> 
> Accessing the web from the language today is a basic feature, and quite more commonly needed than as example complex numbers.
> 
> In Clojure even the Phobos equivalent of the File function loads from the web if you give it an URL instead  of a file path :-) Please help pushing D into this decade.
> 
> Bye,
> bearophile

My original SQLite3 pull request included the SQLite library and had it compiled into Phobos just as zlib. This was turned down from the perspective that bug fixes to SQLite will not require an update to Phobos.

While not everyone voice an opinion no one but myself was arguing to include the library.