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...

Reply via email to