digitalmars.D - Request for review: std.net.isemail
- Jacob Carlborg (12/12) Mar 22 2011 I've now finished the port of Dominic Sayers' PHP is_email function
- dsimcha (18/29) Mar 22 2011 I haven't reviewed the implementation and don't plan to because I don't
- Jacob Carlborg (24/60) Mar 23 2011 The helper functions are private, I just didn't think of that they would...
- Nick Sabalausky (11/26) Mar 23 2011 In that case, I would suggest:
- Jacob Carlborg (4/34) Mar 23 2011 That might be a good idea.
- Jonathan M Davis (11/43) Mar 23 2011 Thanks to a bug, _all_ templated functions are public _regardless_ of wh...
- Jacob Carlborg (4/47) Mar 24 2011 Oh, yeah. I remember that now when you mentioned it.
- Dmitry Olshansky (22/33) Mar 23 2011 I have tried that with my patch for std.regex
- Jonathan M Davis (3/17) Mar 23 2011 If you think that the patch is solid enough, you should submit a pull re...
- Jacob Carlborg (4/41) Mar 24 2011 --
- dsimcha (23/34) Mar 25 2011 Two questions:
- Walter Bright (5/7) Mar 27 2011 I think it looks nice. I'd add a note saying it is derived from Dominic'...
- Jacob Carlborg (4/12) Mar 29 2011 Ok, will do that.
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
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.htmlI 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
On 2011-03-22 23:21, dsimcha wrote:On 3/22/2011 6:04 PM, Jacob Carlborg wrote:The helper functions are private, I just didn't think of that they would show up in the documentation.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.htmlI 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?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
"Jacob Carlborg" <doob me.com> wrote in message news:imcgim$1sk4$1 digitalmars.com...On 2011-03-22 23:21, dsimcha wrote: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.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.
Mar 23 2011
On 2011-03-23 11:46, Nick Sabalausky wrote:"Jacob Carlborg"<doob me.com> wrote in message news:imcgim$1sk4$1 digitalmars.com...That might be a good idea. -- /Jacob CarlborgOn 2011-03-22 23:21, dsimcha wrote: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.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.
Mar 23 2011
On 2011-03-22 23:21, dsimcha wrote: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 DavisOn 3/22/2011 6:04 PM, Jacob Carlborg wrote:The helper functions are private, I just didn't think of that they would show up in the documentation.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.htmlI 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.
Mar 23 2011
On 2011-03-23 18:09, Jonathan M Davis wrote:Oh, yeah. I remember that now when you mentioned it. -- /Jacob CarlborgOn 2011-03-22 23:21, dsimcha wrote: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 DavisOn 3/22/2011 6:04 PM, Jacob Carlborg wrote:The helper functions are private, I just didn't think of that they would show up in the documentation.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.htmlI 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.
Mar 24 2011
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 commentedI 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 bugsYeah, 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
On 23.03.2011 1:04, Jacob Carlborg wrote: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 DavisI'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 commentedI 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.
Mar 23 2011
On 2011-03-23 22:20, Dmitry Olshansky wrote:On 23.03.2011 1:04, Jacob Carlborg wrote:I haven't tried with your patch yet, sorry.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 commentedI 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.-- /Jacob Carlborg* Due to some bugs (4673, 5744) in Phobos this module contains private functions with fixes for these bugsYeah, 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
Mar 24 2011
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.htmlTwo 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
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
On 2011-03-28 07:36, Walter Bright wrote:On 3/22/2011 3:04 PM, Jacob Carlborg wrote:Ok, will do that. -- /Jacob CarlborgI'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 29 2011