On 2011-03-31 00:09, Jacob Carlborg wrote:
> On 3/30/11 10:07 PM, Andrei Alexandrescu wrote:
> > On 3/30/11 2:47 PM, Jonathan M Davis wrote:
> >> I've tried to get Andrei to agree to a style guide a few times, but he's
> >> generally pushed back on it. I definitely think that we should have
> >> one if we
> >> want to actually have a consistent style, but thus far, he hasn't
> >> agreed to
> >> have one.
> > 
> > I think that's not representing my viewpoint quite accurately, but getting to the bottom of whatever misunderstanding was there is not important.
> > 
> > It would be helpful to have a broad style guide. The existing one is a good start and is in fact already observed by much of Phobos (and in particular by most of my code). The problem with writing a more elaborate guide is finding the person(s) with the time and inclination to write a draft, get it vetted by the major contributors, and take it to completion.
> 
> Which one is the existing one, it it available on the DigitalMars site?
> 
> > For my money, just take the first that applies:
> > 
> > - Is it a function name? Use thisStyle.
> > 
> > - Is it a value (be it constant or variable)? Use thisStyle.
> > 
> > - Is it a type? Use ThisStyle.
> > 
> > - Is it a module name? Use this_style.
> > 
> > Beyond naming:
> > 
> > - Define variables as late as possible.
> > 
> > - Define constants and other names scoped appropriately.
> > 
> > - Prefer anonymous temporaries to named values within reason.
> > 
> > - Prefer ? : to if/else within reason.
> > 
> > - Prefer compact, effective code to verbose code within reason. Make every line count.
> 
> I don't like this one. I would say prefer readable code to compact code. Don't be afraid of having long descriptive names of variables and having vertical space in your code. I don't mean you should put three newlines between two function declarations but I usually put a newline before and after statements:
> 
> int a;
> int b;
> 
> foo();
> bar();
> 
> if (a == b)
>     foo();
> 
> else
>     bar();
> 
> int c = 3;
> 
> > Nitpicks that 9 people have 10 opinions of:
> > 
> > - No 2+ empty lines please. Within a function, an empty line at best should be replaced by a comment describing the meaning of the next block.
> > 
> > - Try to fit functions loosely on one editor page.
> > 
> > - Brace on its own line. (In fact I don't care much either way, but this is the way the majority of the code is.) Note that this does cost more vertical space so it somewhat clashes with the previous point.
> > 
> > - Please avoid > 80 columns unless you feel it induces sterility in the long term.
> 
> Do you know how narrow 80 columns look on a wide screen. I think it looks narrow on a regular (non-wide) screen.

I definitely have to agree with you on that one (most everything you said actually), but there are obviously differing opinions and it needs to be discussed. I've put together an initial proposal for a style guide (based on what Phobos has been doing and what's been discussed on the newsgroup) and posted it to the Phobos list. So, discuss away on that thread. Hopefully, we can come to a reasonable consensus.

- Jonathan M Davis

On 2011-03-31 01:12, Jonathan M Davis wrote:
> On 2011-03-30 15:27, Andrei Alexandrescu wrote:
>> On 3/30/11 5:24 PM, bearophile wrote:
>>> Andrei:
>>>> Beyond naming:
>>> Some standard for member attributes? Like m_something, etc? I don't like
>>> this a lot, but having a style guide on this too is useful for Phobos
>>> (and user code).
>>>
>>> Bye,
>>> bearophile
>>
>> I usually prepend private data members with an underscore.
>
> That's what std.datetime and std.file do. Regardless, member variables often
> need a different naming scheme from normal variables thanks to properties that
> have the same name. That can go in the style guide, but I don't think that the
> naming scheme for private member variables or local variables is as important
> as the public ones, since they're part of the API. It would still be good to
> be consistent (like it's good to be consistent with braces), but when it comes
> to names, the primary concern is the public API.
>
> - Jonathan M Davis

I think this is only valid if the member variables have a corresponding getter/setter with the same name.

-- 
/Jacob Carlborg

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.

I'd strongly consider making EmailStatus' public member variables private with getter properties (no setters obviously, since they're const), since you can't even assign to a const struct, and that could be annoying. For instance, you can't really put them in an array very well. By making them only have getter properties, you get the same effect but can still put the struct in arrays or reassign it if you need to. const member variables in classes isn't such a big deal, but I'm disinclined to use them in structs due to the issues that they cause.

I'd be inclined to make EmailStatus' constructor private. I'm not sure that it makes any sense for it to be public. Why would anyone create one rather than using isEmail to create it?

You should probably add a note to the documentation that the DNS check hasn't been implemented yet.

It would probably be a good idea to create a free function which takes an EmailStatusCode and returns the status string, move the logic in the status function to there, and have it call the new function. That way, if a program needed to know what the actual string was without having an e-mail with that particular error, it could get at the string.

If you're going to mention a version number, you might want to make it clearer what the code is based on, since from Phobos' perspective, the version number makes no sense. It _does_ make sense to say that it's based on version X of library Y but not that the module itself is version X - especially when that version is 3+, and it's brand-new.



Nitpicks:

You have tabs and trailing space in the file. You shouldn't have either. And even if we wanted tabs, they were used inconsistently.

Why did you create aliases for front, back, and popFront? That's a really odd thing to do and makes the code harder to understand. I would have considered that to fall in the "useless alias" category and best to be avoided.

As stated before, all of your enums seem to start with a capital letter when the typical thing to do in Phobos at this point is to start with a lowercase letter.

It would be nice if isEmail were broken up given its size, but it does seem like the sort of function which wouldn't break up very easily. The most obvious way to do that would be to create separate functions for each case statement, but given all of the variables involved, that wouldn't work very well. About the only sane way that I could think of doing that would be to create class or struct which held all of the logic and had all of those local variables as member variables. Then each of the cases could be broken up into separate functions. It would probably be declared internal to isEmail, and isEmail would construct one and then get the result out of it. That would allow you to break up the function. However, I'm not at all convinced that that would really be a better solution. It does seem a bit convoluted to do that just to break the function up. If it really helped the function's readability and maintainability then it would be worth it, but it might make it worse. So, I don't really like the size of the function, and that's a way that you could break it up, but unless you really think that that's a good idea, I'd just leave it as is. It's the sort of function that's generally stuck being long.



Overall, it looks pretty good. And as long as it works, it should be fine other than the few API issues that I mentioned, and they're not exactly enormous issues.

- Jonathan M Davis

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.

I'd strongly consider making EmailStatus' public member variables private with getter properties (no setters obviously, since they're const), since you can't even assign to a const struct, and that could be annoying. For instance, you can't really put them in an array very well. By making them only have getter properties, you get the same effect but can still put the struct in arrays or reassign it if you need to. const member variables in classes isn't such a big deal, but I'm disinclined to use them in structs due to the issues that they cause.

I'd be inclined to make EmailStatus' constructor private. I'm not sure that it makes any sense for it to be public. Why would anyone create one rather than using isEmail to create it?

You should probably add a note to the documentation that the DNS check hasn't been implemented yet.

It would probably be a good idea to create a free function which takes an EmailStatusCode and returns the status string, move the logic in the status function to there, and have it call the new function. That way, if a program needed to know what the actual string was without having an e-mail with that particular error, it could get at the string.

If you're going to mention a version number, you might want to make it clearer what the code is based on, since from Phobos' perspective, the version number makes no sense. It _does_ make sense to say that it's based on version X of library Y but not that the module itself is version X - especially when that version is 3+, and it's brand-new.



Nitpicks:

You have tabs and trailing space in the file. You shouldn't have either. And even if we wanted tabs, they were used inconsistently.

Why did you create aliases for front, back, and popFront? That's a really odd thing to do and makes the code harder to understand. I would have considered that to fall in the "useless alias" category and best to be avoided.

As stated before, all of your enums seem to start with a capital letter when the typical thing to do in Phobos at this point is to start with a lowercase letter.

It would be nice if isEmail were broken up given its size, but it does seem like the sort of function which wouldn't break up very easily. The most obvious way to do that would be to create separate functions for each case statement, but given all of the variables involved, that wouldn't work very well. About the only sane way that I could think of doing that would be to create class or struct which held all of the logic and had all of those local variables as member variables. Then each of the cases could be broken up into separate functions. It would probably be declared internal to isEmail, and isEmail would construct one and then get the result out of it. That would allow you to break up the function. However, I'm not at all convinced that that would really be a better solution. It does seem a bit convoluted to do that just to break the function up. If it really helped the function's readability and maintainability then it would be worth it, but it might make it worse. So, I don't really like the size of the function, and that's a way that you could break it up, but unless you really think that that's a good idea, I'd just leave it as is. It's the sort of function that's generally stuck being long.



Overall, it looks pretty good. And as long as it works, it should be fine other than the few API issues that I mentioned, and they're not exactly enormous issues.

- Jonathan M Davis

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 strongly consider making EmailStatus' public member variables private with
> getter properties (no setters obviously, since they're const), since you can't
> even assign to a const struct, and that could be annoying. For instance, you
> can't really put them in an array very well. By making them only have getter
> properties, you get the same effect but can still put the struct in arrays or
> reassign it if you need to. const member variables in classes isn't such a big
> deal, but I'm disinclined to use them in structs due to the issues that they
> cause.

Ok, didn't think about that.

> I'd be inclined to make EmailStatus' constructor private. I'm not sure that it
> makes any sense for it to be public. Why would anyone create one rather than
> using isEmail to create it?

I guess you're right.

> You should probably add a note to the documentation that the DNS check hasn't
> been implemented yet.

Yes.

> It would probably be a good idea to create a free function which takes an
> EmailStatusCode and returns the status string, move the logic in the status
> function to there, and have it call the new function. That way, if a program
> needed to know what the actual string was without having an e-mail with that
> particular error, it could get at the string.

Is this really necessary? Would you ever have the status code without the EmailStatus?

> If you're going to mention a version number, you might want to make it clearer
> what the code is based on, since from Phobos' perspective, the version number
> makes no sense. It _does_ make sense to say that it's based on version X of
> library Y but not that the module itself is version X - especially when that
> version is 3+, and it's brand-new.

Yeah, I kind of most of that text as is.

> Nitpicks:
>
> You have tabs and trailing space in the file. You shouldn't have either. And
> even if we wanted tabs, they were used inconsistently.

Forgot about converting the tabs to spaces? How can I easily find trailing spaces?

> Why did you create aliases for front, back, and popFront? That's a really odd
> thing to do and makes the code harder to understand. I would have considered
> that to fall in the "useless alias" category and best to be avoided.

I like those names better and though it wouldn't matter all the much since they're private.

> As stated before, all of your enums seem to start with a capital letter when
> the typical thing to do in Phobos at this point is to start with a lowercase
> letter.

Yes, didn't know what the naming convention were for enum members when I started.

> It would be nice if isEmail were broken up given its size, but it does seem
> like the sort of function which wouldn't break up very easily. The most
> obvious way to do that would be to create separate functions for each case
> statement, but given all of the variables involved, that wouldn't work very
> well. About the only sane way that I could think of doing that would be to
> create class or struct which held all of the logic and had all of those local
> variables as member variables. Then each of the cases could be broken up into
> separate functions. It would probably be declared internal to isEmail, and
> isEmail would construct one and then get the result out of it. That would
> allow you to break up the function. However, I'm not at all convinced that
> that would really be a better solution. It does seem a bit convoluted to do
> that just to break the function up. If it really helped the function's
> readability and maintainability then it would be worth it, but it might make
> it worse. So, I don't really like the size of the function, and that's a way
> that you could break it up, but unless you really think that that's a good
> idea, I'd just leave it as is. It's the sort of function that's generally
> stuck being long.

I agree with you, it's far too big but I don't know how I could break it up. It would be a lot easier if I've written the function myself but it's just a port of a PHP function.

> Overall, it looks pretty good. And as long as it works, it should be fine
> other than the few API issues that I mentioned, and they're not exactly
> enormous issues.

Thanks.

> - Jonathan M Davis


-- 
/Jacob Carlborg

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'd strongly consider making EmailStatus' public member variables private with getter properties (no setters obviously, since they're const), since you can't even assign to a const struct, and that could be annoying. For instance, you can't really put them in an array very well. By making them only have getter properties, you get the same effect but can still put the struct in arrays or reassign it if you need to. const member variables in classes isn't such a big deal, but I'm disinclined to use them in structs due to the issues that they cause.
> 
> Ok, didn't think about that.
> 
> > I'd be inclined to make EmailStatus' constructor private. I'm not sure that it makes any sense for it to be public. Why would anyone create one rather than using isEmail to create it?
> 
> I guess you're right.
> 
> > You should probably add a note to the documentation that the DNS check hasn't been implemented yet.
> 
> Yes.
> 
> > It would probably be a good idea to create a free function which takes an EmailStatusCode and returns the status string, move the logic in the status function to there, and have it call the new function. That way, if a program needed to know what the actual string was without having an e-mail with that particular error, it could get at the string.
> 
> Is this really necessary? Would you ever have the status code without the EmailStatus?

In most cases, no. However, I can easily believe that a fancier program would care. For instance, anything which wanted to list the various e-mail status codes and their associated strings would need to have the list of EmailStatusCodes and their associated strings.

I don't doubt that few programs will care. But it would be so simple to make it possible to get a string and from its corresponding status code without having an EmailStatus, that I see no reason to not make it possible to do so. It makes the module more flexible, and I expect that if the module is used enough, then a program at some point will want that capability.

> > If you're going to mention a version number, you might want to make it clearer what the code is based on, since from Phobos' perspective, the version number makes no sense. It _does_ make sense to say that it's based on version X of library Y but not that the module itself is version X - especially when that version is 3+, and it's brand-new.
> 
> Yeah, I kind of most of that text as is.
> 
> > Nitpicks:
> > 
> > You have tabs and trailing space in the file. You shouldn't have either. And even if we wanted tabs, they were used inconsistently.
> 
> Forgot about converting the tabs to spaces? How can I easily find trailing spaces?

We have only been using spaces in Phobos - no tabs - and I don't expect that to change, so code in Phobos shouldn't have tabs in it. Mixing tabs and spaces tends to ruin formatting and cause problems. And trailing spaces are just wasted characters.

Personally, I have vim set up to always highlight any tabs as well as trailing whitespace, so I notice it immediately - which is why I noticed in this case.

Typically, there's a way to get code editors to show extra whitespace and/or remove it when saving the file. They also typically have a way to convert tabs to spaces. How that works depends in your editor though depends on your editor.

If you're using vim, then :retab would turn the tabs into spaces and the command :s/\s\+$// will remove all trailing whitespace (and presumably that can be converted into an appropriate sed command, but I'm not well-versed in sed).

> > Why did you create aliases for front, back, and popFront? That's a really odd thing to do and makes the code harder to understand. I would have considered that to fall in the "useless alias" category and best to be avoided.
> 
> I like those names better and though it wouldn't matter all the much since they're private.

It doesn't matter as much, but I think that it still matters. Using unnecessary aliases makes it harder to read code - especially when they're aliases for well-known symbols. front, popFront, and back are standard Phobos functions which are part of one of the core concepts in Phobos, and they're what the rest of Phobos uses. You can't get much more standard than that. tstring at least makes the code shorter, and it's actually listed before it's used (unlike the other aliases), and I'm still torn on whether it should be there. But aliases which are simple renamings for standard Phobos functions are completely unnecessary and harm code maintainability for anyone else who reads the code.

- Jonathan M Davis

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.

>>> I'd strongly consider making EmailStatus' public member variables private
>>> with getter properties (no setters obviously, since they're const),
>>> since you can't even assign to a const struct, and that could be
>>> annoying. For instance, you can't really put them in an array very well.
>>> By making them only have getter properties, you get the same effect but
>>> can still put the struct in arrays or reassign it if you need to. const
>>> member variables in classes isn't such a big deal, but I'm disinclined
>>> to use them in structs due to the issues that they cause.
>>
>> Ok, didn't think about that.
>>
>>> I'd be inclined to make EmailStatus' constructor private. I'm not sure
>>> that it makes any sense for it to be public. Why would anyone create one
>>> rather than using isEmail to create it?
>>
>> I guess you're right.
>>
>>> You should probably add a note to the documentation that the DNS check
>>> hasn't been implemented yet.
>>
>> Yes.
>>
>>> It would probably be a good idea to create a free function which takes an
>>> EmailStatusCode and returns the status string, move the logic in the
>>> status function to there, and have it call the new function. That way,
>>> if a program needed to know what the actual string was without having an
>>> e-mail with that particular error, it could get at the string.
>>
>> Is this really necessary? Would you ever have the status code without
>> the EmailStatus?
>
> In most cases, no. However, I can easily believe that a fancier program would
> care. For instance, anything which wanted to list the various e-mail status
> codes and their associated strings would need to have the list of
> EmailStatusCodes and their associated strings.
>
> I don't doubt that few programs will care. But it would be so simple to make
> it possible to get a string and from its corresponding status code without
> having an EmailStatus, that I see no reason to not make it possible to do so.
> It makes the module more flexible, and I expect that if the module is used
> enough, then a program at some point will want that capability.

Ok, I'll add a function, any idea for a good name?

>>> If you're going to mention a version number, you might want to make it
>>> clearer what the code is based on, since from Phobos' perspective, the
>>> version number makes no sense. It _does_ make sense to say that it's
>>> based on version X of library Y but not that the module itself is
>>> version X - especially when that version is 3+, and it's brand-new.
>>
>> Yeah, I kind of most of that text as is.
>>
>>> Nitpicks:
>>>
>>> You have tabs and trailing space in the file. You shouldn't have either.
>>> And even if we wanted tabs, they were used inconsistently.
>>
>> Forgot about converting the tabs to spaces? How can I easily find
>> trailing spaces?
>
> We have only been using spaces in Phobos - no tabs - and I don't expect that
> to change, so code in Phobos shouldn't have tabs in it. Mixing tabs and spaces
> tends to ruin formatting and cause problems. And trailing spaces are just
> wasted characters.
>
> Personally, I have vim set up to always highlight any tabs as well as trailing
> whitespace, so I notice it immediately - which is why I noticed in this case.
>
> Typically, there's a way to get code editors to show extra whitespace and/or
> remove it when saving the file. They also typically have a way to convert tabs
> to spaces. How that works depends in your editor though depends on your
> editor.
>
> If you're using vim, then :retab would turn the tabs into spaces and the
> command :s/\s\+$// will remove all trailing whitespace (and presumably that
> can be converted into an appropriate sed command, but I'm not well-versed in
> sed).

I think it's easier to work with tabs than spaces so I planned to work with tabs and than the last thing I would do before submitting a pull request would be to convert the tabs spaces, just forgot that. I just did a simple search and replace in my editor with a regular expression to remove the trailing spaces.

>>> Why did you create aliases for front, back, and popFront? That's a really
>>> odd thing to do and makes the code harder to understand. I would have
>>> considered that to fall in the "useless alias" category and best to be
>>> avoided.
>>
>> I like those names better and though it wouldn't matter all the much
>> since they're private.
>
> It doesn't matter as much, but I think that it still matters. Using
> unnecessary aliases makes it harder to read code - especially when they're
> aliases for well-known symbols. front, popFront, and back are standard Phobos
> functions which are part of one of the core concepts in Phobos, and they're
> what the rest of Phobos uses. You can't get much more standard than that.
> tstring at least makes the code shorter, and it's actually listed before it's
> used (unlike the other aliases), and I'm still torn on whether it should be
> there. But aliases which are simple renamings for standard Phobos functions
> are completely unnecessary and harm code maintainability for anyone else who
> reads the code.

I've already removed these aliases (not tstring).

> - Jonathan M Davis


-- 
/Jacob Carlborg

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

On Sun, 03 Apr 2011 09:49:44 -0400, Andrei Alexandrescu <SeeWebsiteForEmail@erdani.org> 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

Also, static assert doesn't compose with is(typeof(...)) and __traits(compiles,...). (It stops the compiler if it's every evaluated) I'd recommend simply putting a string on an empty line over static assert:

static assert(...,"My error message");

static if(...) "My error message";

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.

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

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

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

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

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

* Line 189: enforce?

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

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

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

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


Andrei