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


Yeah, there are always not enough bugfixes to pull new module it seems ;).


 * 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


 Yeah, there are always not enough bugfixes to pull new module it seems ;).


 * 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