On 3/29/11 4:06 PM, Jacob Carlborg wrote:
> I've made a few minor changes:
>
> * Renamed EmailStatusCode.Off -> None and On -> Any

I suggest we follow the nascent convention that enum values start with a lowercase letter.

Andrei

On 2011-04-03 15:49, Andrei Alexandrescu wrote:
> On 4/3/11 6:38 AM, Jacob Carlborg wrote:
>> On 2011-04-02 23:33, Jonathan M Davis wrote:
>>> On 2011-04-02 09:26, Jacob Carlborg wrote:
>>>> On 2011-04-01 11:01, Jonathan M Davis wrote:
>>>>> On 2011-03-29 14:06, 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
>>>>>
>>>>> Shouldn't isEmail have a template constraint which verifies that the
>>>>> e-mail variable is a valid string type? Also, it would probably be
>>>>> better to use Char instead of T for the type. It's clearer that way. I
>>>>> believe that that's what std.string does.
>>>>
>>>> That's a good idea.
>>>
>>> I'd say that pretty much every template should have a template
>>> constraint on
>>> it. I'm not sure that I've ever seen a template which was flexible
>>> enough that
>>> it would compile with anything you gave it, and if there's an argument
>>> that
>>> you can give it which won't compile with it, it should fail the template
>>> constraints, otherwise the error messags are much harder to understand
>>> and
>>> track down.
>>
>> I've added a static assert with a custom error message. I think static
>> asserts can give better error messages than template constraints.
>
> Please use a template constraint. This is the right thing to do and we
> use it throughout Phobos.
>
> Thanks,
>
> Andrei

Ok, if you say so but the error message won't be as good as with a static assert.

-- 
/Jacob Carlborg

On 2011-04-03 20:22, 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
>
> I suggest we follow the nascent convention that enum values start with a
> lowercase letter.
>
> Andrei

Already fixed.

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

On 2011-04-03 13:13, Jacob Carlborg wrote:
> 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?

Andrei has been pushing for this, because it makes the calls to the function easier to read. If it's CheckDns.yes, then it's clear what you wanted, whereas if it were true, you'd have to look up whether true meant that it's supposed to check or skip checking. In some cases, that's more useful than others, but it's already becoming fairly common in Phobos that such explicit enums are used rather than bools.

- Jonathan M Davis

On 4/3/11 4:38 PM, Jonathan M Davis wrote:
> On 2011-04-03 13:13, Jacob Carlborg wrote:
>> 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?
>
> Andrei has been pushing for this, because it makes the calls to the function
> easier to read. If it's CheckDns.yes, then it's clear what you wanted, whereas
> if it were true, you'd have to look up whether true meant that it's supposed
> to check or skip checking. In some cases, that's more useful than others, but
> it's already becoming fairly common in Phobos that such explicit enums are
> used rather than bools.

This becomes painfully obvious in calls with more than one Boolean-encoded categorical argument.

auto r = getWidget("widget", true, false);

The topic is discussed in the book Code Complete and I suppose in many other style guides.


Andrei