www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Request for review: std.net.isemail

reply Jacob Carlborg <doob me.com> writes:
I've now finished the port of Dominic Sayers' PHP is_email function 
(http://www.dominicsayers.com/isemail) and sending it for review.

A few comments:

* Due to limitations in std.regex some unit tests fail and are out commented

* Due to some bugs (4673, 5744) in Phobos this module contains private 
functions with fixes for these bugs

* The DNS check is not implemented resulting in a few out commented unit 
tests

Github: https://github.com/jacob-carlborg/phobos/tree/isemail
Documentation: http://dl.dropbox.com/u/18386187/isemail.html

-- 
/Jacob Carlborg
Mar 22 2011
next sibling parent reply dsimcha <dsimcha yahoo.com> writes:
On 3/22/2011 6:04 PM, Jacob Carlborg wrote:
 I've now finished the port of Dominic Sayers' PHP is_email function
 (http://www.dominicsayers.com/isemail) and sending it for review.

 A few comments:

 * Due to limitations in std.regex some unit tests fail and are out
 commented

 * Due to some bugs (4673, 5744) in Phobos this module contains private
 functions with fixes for these bugs

 * The DNS check is not implemented resulting in a few out commented unit
 tests

 Github: https://github.com/jacob-carlborg/phobos/tree/isemail
 Documentation: http://dl.dropbox.com/u/18386187/isemail.html

I haven't reviewed the implementation and don't plan to because I don't know enough about what issues might arise with something like this to provide good feedback. My major comment about the API is that helper functions like firstChar(), max(), substr(), compareFirstN(), grep() and get() should be made private and not included in documentation. If you think they might be generally useful, they can be made public and documented once we figure out where they belong. They definitely don't belong in the API of a module for email address validation, though. Also a few questions regarding status codes: It looks like the status codes are not mutually exclusive. 1. How are multiple errors handled? Are the status codes or'd? Is one returned arbitrarily? 2. Does the multiple status codes issue matter for real use cases? 3. What are EmailStatusCode.On and EmailStatusCode.Off? Other than that, it looks pretty good to me. It solves a simple (at the conceptual level if not the implementation level) problem with a simple API.
Mar 22 2011
next sibling parent reply Jacob Carlborg <doob me.com> writes:
On 2011-03-22 23:21, dsimcha wrote:
 On 3/22/2011 6:04 PM, Jacob Carlborg wrote:
 I've now finished the port of Dominic Sayers' PHP is_email function
 (http://www.dominicsayers.com/isemail) and sending it for review.

 A few comments:

 * Due to limitations in std.regex some unit tests fail and are out
 commented

 * Due to some bugs (4673, 5744) in Phobos this module contains private
 functions with fixes for these bugs

 * The DNS check is not implemented resulting in a few out commented unit
 tests

 Github: https://github.com/jacob-carlborg/phobos/tree/isemail
 Documentation: http://dl.dropbox.com/u/18386187/isemail.html

I haven't reviewed the implementation and don't plan to because I don't know enough about what issues might arise with something like this to provide good feedback. My major comment about the API is that helper functions like firstChar(), max(), substr(), compareFirstN(), grep() and get() should be made private and not included in documentation. If you think they might be generally useful, they can be made public and documented once we figure out where they belong. They definitely don't belong in the API of a module for email address validation, though.

The helper functions are private, I just didn't think of that they would show up in the documentation.
 Also a few questions regarding status codes: It looks like the status
 codes are not mutually exclusive.

 1. How are multiple errors handled? Are the status codes or'd? Is one
 returned arbitrarily?

I think the original PHP function returned an array of status codes. Hmm, I wonder what happened to this. The D version just returns the largest status code.
 2. Does the multiple status codes issue matter for real use cases?

I have no idea. I think most of the users just want to know if the email address is valid or not.
 3. What are EmailStatusCode.On and EmailStatusCode.Off?

I had some problem figuring out how I wanted to solve this. In the PHP version the function takes a parameter, errorLevel, indicating what error level you want. Any status code above will be returned as-is and status codes below will be returned as "valid". In addition to this you can pass "true" or "false". If "true", any status code except "valid" will be considered invalid. If "false", the function just returns "true" or "false" rather than an integer error or warning. The problem I had was that since D is statically typed you cannot mix integer and boolean values in one parameter, specially not if the integer parameter is an enum. Since I wanted to be able to pass both a EmailStatusCode value as errorLevel and "true" and "false" I added EmailStatusCode.On and EmailStatusCode.Off representing "true" and "false". I should document this.
 Other than that, it looks pretty good to me. It solves a simple (at the
 conceptual level if not the implementation level) problem with a simple
 API.

Thanks. -- /Jacob Carlborg
Mar 23 2011
next sibling parent reply "Nick Sabalausky" <a a.a> writes:
"Jacob Carlborg" <doob me.com> wrote in message 
news:imcgim$1sk4$1 digitalmars.com...
 On 2011-03-22 23:21, dsimcha wrote:

 3. What are EmailStatusCode.On and EmailStatusCode.Off?

I had some problem figuring out how I wanted to solve this. In the PHP version the function takes a parameter, errorLevel, indicating what error level you want. Any status code above will be returned as-is and status codes below will be returned as "valid". In addition to this you can pass "true" or "false". If "true", any status code except "valid" will be considered invalid. If "false", the function just returns "true" or "false" rather than an integer error or warning. The problem I had was that since D is statically typed you cannot mix integer and boolean values in one parameter, specially not if the integer parameter is an enum. Since I wanted to be able to pass both a EmailStatusCode value as errorLevel and "true" and "false" I added EmailStatusCode.On and EmailStatusCode.Off representing "true" and "false". I should document this.

In that case, I would suggest: - Rename EmailStatusCode.On to EmailStatusCode.Any - Remove EmailStatusCode.Off, and split isEmail into two overloads: EmailStatus isEmail(T)(const(T)[] email, bool checkDNS, EmailStatusCode errorLevel); and bool isEmail(T)(const(T)[] email, bool checkDNS = false); That second one would be the equivalent of passing "false" into the PHP version.
Mar 23 2011
parent Jacob Carlborg <doob me.com> writes:
On 2011-03-23 11:46, Nick Sabalausky wrote:
 "Jacob Carlborg"<doob me.com>  wrote in message
 news:imcgim$1sk4$1 digitalmars.com...
 On 2011-03-22 23:21, dsimcha wrote:

 3. What are EmailStatusCode.On and EmailStatusCode.Off?

I had some problem figuring out how I wanted to solve this. In the PHP version the function takes a parameter, errorLevel, indicating what error level you want. Any status code above will be returned as-is and status codes below will be returned as "valid". In addition to this you can pass "true" or "false". If "true", any status code except "valid" will be considered invalid. If "false", the function just returns "true" or "false" rather than an integer error or warning. The problem I had was that since D is statically typed you cannot mix integer and boolean values in one parameter, specially not if the integer parameter is an enum. Since I wanted to be able to pass both a EmailStatusCode value as errorLevel and "true" and "false" I added EmailStatusCode.On and EmailStatusCode.Off representing "true" and "false". I should document this.

In that case, I would suggest: - Rename EmailStatusCode.On to EmailStatusCode.Any - Remove EmailStatusCode.Off, and split isEmail into two overloads: EmailStatus isEmail(T)(const(T)[] email, bool checkDNS, EmailStatusCode errorLevel); and bool isEmail(T)(const(T)[] email, bool checkDNS = false); That second one would be the equivalent of passing "false" into the PHP version.

That might be a good idea. -- /Jacob Carlborg
Mar 23 2011
prev sibling parent Jacob Carlborg <doob me.com> writes:
On 2011-03-23 18:09, Jonathan M Davis wrote:
 On 2011-03-22 23:21, dsimcha wrote:
 On 3/22/2011 6:04 PM, Jacob Carlborg wrote:
 I've now finished the port of Dominic Sayers' PHP is_email function
 (http://www.dominicsayers.com/isemail) and sending it for review.

 A few comments:

 * Due to limitations in std.regex some unit tests fail and are out
 commented

 * Due to some bugs (4673, 5744) in Phobos this module contains private
 functions with fixes for these bugs

 * The DNS check is not implemented resulting in a few out commented unit
 tests

 Github: https://github.com/jacob-carlborg/phobos/tree/isemail
 Documentation: http://dl.dropbox.com/u/18386187/isemail.html

I haven't reviewed the implementation and don't plan to because I don't know enough about what issues might arise with something like this to provide good feedback. My major comment about the API is that helper functions like firstChar(), max(), substr(), compareFirstN(), grep() and get() should be made private and not included in documentation. If you think they might be generally useful, they can be made public and documented once we figure out where they belong. They definitely don't belong in the API of a module for email address validation, though.

The helper functions are private, I just didn't think of that they would show up in the documentation.

Thanks to a bug, _all_ templated functions are public _regardless_ of what you mark them as (I recall a different bug report for it, but here's one: http://d.puremagic.com/issues/show_bug.cgi?id=2775 ). So, if you put documentation on private templated functions (be they directly templated or member functions of a templated type), you have to make sure that they aren't actually ddoc comments (so they start with /+ instead of /++ or /* instead of /**), otherwise, they end up in the documentation. Unfortunately, regardless of whether you document them, they could still be called in spite of the fact that they're supposed to be private, but one would hope that no one would do that. Hopefully the bug gets fixed at some point though. - Jonathan M Davis

Oh, yeah. I remember that now when you mentioned it. -- /Jacob Carlborg
Mar 24 2011
prev sibling parent Jonathan M Davis <jmdavisProg gmx.com> writes:
 On 2011-03-22 23:21, dsimcha wrote:
 On 3/22/2011 6:04 PM, Jacob Carlborg wrote:
 I've now finished the port of Dominic Sayers' PHP is_email function
 (http://www.dominicsayers.com/isemail) and sending it for review.
 
 A few comments:
 
 * Due to limitations in std.regex some unit tests fail and are out
 commented
 
 * Due to some bugs (4673, 5744) in Phobos this module contains private
 functions with fixes for these bugs
 
 * The DNS check is not implemented resulting in a few out commented unit
 tests
 
 Github: https://github.com/jacob-carlborg/phobos/tree/isemail
 Documentation: http://dl.dropbox.com/u/18386187/isemail.html

I haven't reviewed the implementation and don't plan to because I don't know enough about what issues might arise with something like this to provide good feedback. My major comment about the API is that helper functions like firstChar(), max(), substr(), compareFirstN(), grep() and get() should be made private and not included in documentation. If you think they might be generally useful, they can be made public and documented once we figure out where they belong. They definitely don't belong in the API of a module for email address validation, though.

The helper functions are private, I just didn't think of that they would show up in the documentation.

Thanks to a bug, _all_ templated functions are public _regardless_ of what you mark them as (I recall a different bug report for it, but here's one: http://d.puremagic.com/issues/show_bug.cgi?id=2775 ). So, if you put documentation on private templated functions (be they directly templated or member functions of a templated type), you have to make sure that they aren't actually ddoc comments (so they start with /+ instead of /++ or /* instead of /**), otherwise, they end up in the documentation. Unfortunately, regardless of whether you document them, they could still be called in spite of the fact that they're supposed to be private, but one would hope that no one would do that. Hopefully the bug gets fixed at some point though. - Jonathan M Davis
Mar 23 2011
prev sibling next sibling parent reply Dmitry Olshansky <dmitry.olsh gmail.com> writes:
On 23.03.2011 1:04, Jacob Carlborg wrote:
 I've now finished the port of Dominic Sayers' PHP is_email function 
 (http://www.dominicsayers.com/isemail) and sending it for review.

 A few comments:

 * Due to limitations in std.regex some unit tests fail and are out 
 commented

I have tried that with my patch for std.regex (seehttp://d.puremagic.com/issues/show_bug.cgi?id=5673 ). Actually, std.regex future is worth of another NG thread altogether. BTW if there anything wrong with that patch, *please* submit any kind of feedback. All of commented out regex unittests pass except these two. Line 824: assert(`test [IPv6:::3333:4444:5555:6666:7777:8888]`.isEmail(false, EmailStatusCode.On).statusCode == EmailStatusCode.Rfc5321AddressLiteral); Line 826: assert(`test [IPv6:::]`.isEmail(false, EmailStatusCode.On).statusCode == EmailStatusCode.Rfc5321AddressLiteral); Both status message is reportedly "IPv6 address starts with a single colon" haven't looked into that deeper. Speaking of module itself, overall, I'd say it already has simple and convenient interface. I concur with others, that none of helper artifacts should slip into the docs though, they are just that - an implementation detail for the most part.
 * Due to some bugs (4673, 5744) in Phobos this module contains private 
 functions with fixes for these bugs

 * The DNS check is not implemented resulting in a few out commented 
 unit tests

 Github: https://github.com/jacob-carlborg/phobos/tree/isemail
 Documentation: http://dl.dropbox.com/u/18386187/isemail.html

-- Dmitry Olshansky
Mar 23 2011
next sibling parent Jonathan M Davis <jmdavisProg gmx.com> writes:
 On 23.03.2011 1:04, Jacob Carlborg wrote:
 I've now finished the port of Dominic Sayers' PHP is_email function
 (http://www.dominicsayers.com/isemail) and sending it for review.
 
 A few comments:
 
 * Due to limitations in std.regex some unit tests fail and are out
 commented

I have tried that with my patch for std.regex (seehttp://d.puremagic.com/issues/show_bug.cgi?id=5673 ). Actually, std.regex future is worth of another NG thread altogether. BTW if there anything wrong with that patch, *please* submit any kind of feedback.

If you think that the patch is solid enough, you should submit a pull request for it: https://github.com/D-Programming-Language/phobos - Jonathan M Davis
Mar 23 2011
prev sibling parent Jacob Carlborg <doob me.com> writes:
On 2011-03-23 22:20, Dmitry Olshansky wrote:
 On 23.03.2011 1:04, Jacob Carlborg wrote:
 I've now finished the port of Dominic Sayers' PHP is_email function
 (http://www.dominicsayers.com/isemail) and sending it for review.

 A few comments:

 * Due to limitations in std.regex some unit tests fail and are out
 commented

I have tried that with my patch for std.regex (seehttp://d.puremagic.com/issues/show_bug.cgi?id=5673 ). Actually, std.regex future is worth of another NG thread altogether. BTW if there anything wrong with that patch, *please* submit any kind of feedback. All of commented out regex unittests pass except these two. Line 824: assert(`test [IPv6:::3333:4444:5555:6666:7777:8888]`.isEmail(false, EmailStatusCode.On).statusCode == EmailStatusCode.Rfc5321AddressLiteral); Line 826: assert(`test [IPv6:::]`.isEmail(false, EmailStatusCode.On).statusCode == EmailStatusCode.Rfc5321AddressLiteral); Both status message is reportedly "IPv6 address starts with a single colon" haven't looked into that deeper.

I haven't tried with your patch yet, sorry.
 Speaking of module itself, overall, I'd say it already has simple and
 convenient interface.
 I concur with others, that none of helper artifacts should slip into the
 docs though, they are just that - an implementation detail for the most
 part.

 * Due to some bugs (4673, 5744) in Phobos this module contains private
 functions with fixes for these bugs

 * The DNS check is not implemented resulting in a few out commented
 unit tests

 Github: https://github.com/jacob-carlborg/phobos/tree/isemail
 Documentation: http://dl.dropbox.com/u/18386187/isemail.html


-- /Jacob Carlborg
Mar 24 2011
prev sibling next sibling parent dsimcha <dsimcha yahoo.com> writes:
On 3/22/2011 6:04 PM, Jacob Carlborg wrote:
 I've now finished the port of Dominic Sayers' PHP is_email function
 (http://www.dominicsayers.com/isemail) and sending it for review.

 A few comments:

 * Due to limitations in std.regex some unit tests fail and are out
 commented

 * Due to some bugs (4673, 5744) in Phobos this module contains private
 functions with fixes for these bugs

 * The DNS check is not implemented resulting in a few out commented unit
 tests

 Github: https://github.com/jacob-carlborg/phobos/tree/isemail
 Documentation: http://dl.dropbox.com/u/18386187/isemail.html

Two questions: 1. When is this module scheduled for a vote? The review process has fizzled out. This may be because the module is so simple that noone has any major issues. It solves a simple problem (at a conceptual level--this is not meant to trivialize the detail work of implementing the standard correctly) with a correspondingly simple interface, so I'm already a "yes". It may also be because there's no urgency since there's no looming vote date. 2. Is voting for one module allowed to run concurrently with reviewing for another? I'm thinking about how we'll manage the review queue while voting on this module. I propose we keep review open through March 28 (one full week of review), then vote. If there are major suggestions that Jacob needs extra time to consider or implement, the review can be stashed (like my std.parallelism review was) to allow time for this. Otherwise, voting runs from the 29th through April 4. Next in queue would be an un-stashing of std.parallelism. I propose that we have one additional week of review for this concurrently with the vote for std.net.isemail, then have the std.parallelism vote right after the isemail vote. std.parallelism has been through a lot of review already, so despite its complexity it probably doesn't need more than a week of additional review.
Mar 25 2011
prev sibling parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 3/22/2011 3:04 PM, Jacob Carlborg wrote:
 I've now finished the port of Dominic Sayers' PHP is_email function
 (http://www.dominicsayers.com/isemail) and sending it for review.

I think it looks nice. I'd add a note saying it is derived from Dominic's php code, I'd add your name to the copyright for the derived D version. The Link: should be placed under the References: instead. It'll be a valuable addition to Phobos, and a good start on net support.
Mar 27 2011
parent Jacob Carlborg <doob me.com> writes:
On 2011-03-28 07:36, Walter Bright wrote:
 On 3/22/2011 3:04 PM, Jacob Carlborg wrote:
 I've now finished the port of Dominic Sayers' PHP is_email function
 (http://www.dominicsayers.com/isemail) and sending it for review.

I think it looks nice. I'd add a note saying it is derived from Dominic's php code, I'd add your name to the copyright for the derived D version. The Link: should be placed under the References: instead. It'll be a valuable addition to Phobos, and a good start on net support.

Ok, will do that. -- /Jacob Carlborg
Mar 29 2011