On Thursday, 15 December 2011 at 07:46:56 UTC, Jonathan M Davis
wrote:
On Friday, December 02, 2011 23:26:10 dsimcha wrote:
I volunteered ages ago to manage the review for the second
round of
Jonas Drewsen's CURL wrapper. After the first round it was
decided
that, after a large number of minor issues were fixed, a
second round
would be necessary.
Significant open issues:
1. Should libcurl be bundled with DMD on Windows?
I'd argue that it should be a separate download but that it
should definitely be provided.
2. etc.curl, std.curl, or std.net.curl? (We had a vote a
while back
but it was buried deep in a thread and a lot of people may
have missed
it:
http://www.easypolls.net/poll.html?p=4ebd3219011eb0e4518d35ab )
If we're going to follow a policy that wrappers around 3rd
party libraries go in etc, then it should be etc.curl.
Otherwise, I think that it should be std.net.curl.
I'd argue that AutoConnection should be AutoConnect, because
it's shorter and just as informative.
AutoConnect sounds like a command to connection automatically
which might be confusing since that is not what it does.
Therefore I went with AutoConnection which I still believe is
better.
I'd argue that the acronyms should be in all caps if
camelcasing would require that the first letter of the acronym
be capitalized and all lower case if camelcasing would require
that the first letter of the acronym be lower case. We're not
completely consistent in how we using acronyms in names in
Phobos, but I believe that we primarily follow that rule when
putting them in symbol names. So, for instance, it would be
HTTP, FTP, and SMTP rather than Http, Ftp, and Smtp.
I like Http better as personal taste. But if HTTP is preferred
then I'll do that. Actually I think I will make a pull request to
extend the dlang.org/dstyle.html doc with and example that we can
point to when someone asks about styling. I've spend too much
time restyling because there is no such style doc already and
people are complaining about style in reviews anyway.
I'd rename T to C or Char in the template parameters, since
it's a character type rather than a generic type. Not a huge
deal, but I think that it makes the code quicker to understand.
I've named it T because it can also be ubyte. In that case C for
Char is confusing.
Why are some parameters const(char)[] and others string? Why
not make them all const(char)[] or even actually templatize
them on character type (one per parameter which is a string)?
The parameters that are const(char)[] will accept both string and
char[] which is what I want because libcurl make an internal copy
of it anyway. If I typed it as string then you would have to idup
a char[] for the parameter without any reason.
The parameters that are string is internally passed to functions
(in other std modules) that only accept strings. I could accept
const(char)[] and idup it in order to have consistant parameter
types but that gives an unnecessary overhead.
Shorten the URL in your examples. Its long length increases the
risk of making the examples too wide. It's a made up URL, so
there's no need for it to be that long. Something like
foo.bar.com is probably more than enough.
Actually the long urls point to a google appspot app that fits
the example. I've done it this way so that people can just
copy/paste the example and have something working. Not too many
people have a test HTTP POST server setup for a quick test AFAIK.
Why does byLine use '\x0a' instead of '\n'? '\n' is clear,
whereas most people will have to look up what '\x0a' is, so I
really think that it should be '\n'.
I looked at how it was done elsewhere in phobos and did like that:
http://dlang.org/phobos/std_stdio.html#byLine
But I agree with you that \n is better and will change it.
Also, if byLine is going to return an internal range type
instead of having an externally defined one, its documentation
needs to explain what Char is used for. As it stands, it looks
like a pointless template parameter. To do that, the return
section should probably say something like "A range of
Char[]..." All of this goes for byLineAsync as well.
Ok.
In general, the functions should do a better job of documenting
their parameters. Many of them don't document their parameters
at all. For instance, while it's fairly easy to guess what
keepTerminator and terminator are used for, there is no
explanation about them whatsoever in either byLine or
byLineAsync's documentation.
Again I looked at phobos to see what was current the style:
http://dlang.org/phobos/std_stdio.html#byLine
I'll include some more detail though.
If any of the range return types of these functions have
non-standard functions on them, they need to be properly
documented. e.g. wait(Duration) is mentioned in passing in
byLineAsync's documentation, but no explanation for its purpose
is given. The documentation needs to explain everything the
programmer needs to know about the return types. For most
ranges, it's enough to say that the return type is a range of
X, but if the return type has anything other than the standard
range functions, then just saying that the function returns a
range of X is not enough. The extra functions must be listed
and explained.
This is what is currently documented: "If no data is available
and the main thread access the range it will block
until data becomes available. An exception to this is the $(D
wait(Duration))
method on the range. This method will wait at maximum for the
specified
duration and return true if data is available."
I guess that explains it?
I'd warn you that the fact that the documentation for Protocol
is showing up in the documentation is likely a bug, since
Protocol is private - probably the bug that all templates are
currently public even if they're marked private. So, either
Protocol needs to be public (or maybe protected), or it's going
to disappear from the documentation eventually. Another option
would be to do something similar to
std.container.TotalContainer and declare a struct which is
intended only for documentation (probably in a version(StdDdoc)
block), and put all of the protocol documenation on _it_
instead of the mixin.
Ok. didn't know about the private template bug. I really think it
is a bug in ddoc that it should be able to insert mixed in docs
ie. the Protocol documentation should be mixed into the Http
documentation.
Anyway - I don't see the TotalContainer docs showing in the
generated html?
Another workaround is to manually copy the Protocol docs into the
HTTP/FTP/SMTP structs inside a version(StdDdoc).
Also, Protocol needs a better explanation as to what it's
actually for. "Mixin template for all supported curl protocols"
doesn't really tell me anything other than the fact that it
relates to the supported protocols somehow. Looking at the
source, it seems clear enough, but not from the documentation -
especially when the documentation seems to imply that it's
specific to the Http struct.
I'll remove the HTTP hint and improve the doc.
onReceiveHeader has const(char)[] parameters but mentions
char[] in its documentation. The meaning is clear enough, but
it would be more correct to say const(char)[] or to just
mention the variable names explicitly.
ok.
It would be nice if Smtp.mailTo was variadic - i.e.
mailTo(string[] recipients...). All of the current use cases
for it would be exactly the same with the addition of being
able to do something like mailTo(str1, str2, str3).
ok
The exception type's constructors should look like this:
nothrow this(string msg, string file = __FILE__, size_t line
= __LINE__, Throwable next = null)
{
super(msg, file, line, next);
}
As it stands, the line number will be completely wrong. You can
just copy std.string.StringException and rename it. And please
use that exact signature. Don't make it take a const(char)[].
Exception requires a string, and with the constructor taking
const(char)[], it's going to needlessly idup every string that
gets passed into it. The code which calls it can idup if it has
a char[] instead of an immutable(int)[]. Having it take a
const(char)[] adds no value and is less efficient. The same
goes for any other function which is always going to idup its
const(char)[] argument. Just make it take a string.
ok
Why do CURLoption and CURLcode have CURL in uppercase? That
doesn't match the rest of the module. I'd argue that they
should be CurlOption and CurlCode. To make matters even
weirder, your examples appear to use CurlOption but the
declarations use CURLoption. They should be consistent.
ok
Thanks for the comments.
-Jonas
