Review of std.net.isemail part 2

[Jacob Carlborg]

On 2011-04-03 20:09, Andrei Alexandrescu wrote:
> On 3/29/11 4:06 PM, Jacob Carlborg wrote:
>> I've made a few minor changes:
>>
>> * Renamed EmailStatusCode.Off -> None and On -> Any
>> * Added and clarified the documentation for EmailStatusCode.Any and None
>> * Updated the documentation
>>
>> Github: https://github.com/jacob-carlborg/phobos/tree/isemail
>> Docs: http://dl.dropbox.com/u/18386187/isemail.html
>
> Overall I think the module is in good shape for admission, with the nits
> below. Here is my review:
>
> * First doc sentence: "To validate..." is grammatically mistaken
> (doesn't have a predicate). Normally I'd just note that and fix later,
> but this is the first sentence so I guess it gets a higher priority.

I'll fix that.

> * Copyright: please look up and use the comment convention used
> elsewhere in Phobos.

Ok.

> * Instead of using a bool for checkDns I suggest enum CheckDns { no, yes
> } which is used in other parts of Phobos.

Why is this better? This is just like the pointless aliases you want to 
avoid. Why do we even have a bool type in the language if we're going to 
replace it with enums all the time?

> * Line 67: Please use template constraints instead of static assert.

Ok, but the error message won't be as good as with the static assert.

> * Line 79 and others: I wish there were no empty line before the "else"
> clause...

I'm not changing that, if you want to do it fine, go ahead.

> * Line 86: There is a constant Threshold that doesn't follow common
> conventions.

Forgot that one.

> * Line 189: enforce?

Sure.

> * Line 235: At best 33, 126, and 10 were symbolic constants or otherwise
> explained.

I'll do something about this.

> * Line 920: EmailStatus should be either private or documented.

EmailStatus is documented, note the three slashes, which is a doc comment.

> * Line 1316: Why is Token a struct with an enum in it instead of an
> enum? (Also the static: label is unnecessary.)

Hmm, I think I thought that if Token was an enum it wouldn't be 
implicitly convertable to string but it seems I was wrong.

> * Line 1357: Why is there a need for conversion to int? Wouldn't dchar
> work well?

I just did a straight port of the PHP code and that's how it looked. 
I'll see if I can do something about it.

> Andrei

-- 
/Jacob Carlborg