feO2x / Light.GuardClauses

A lightweight .NET library for expressive Guard Clauses.
MIT License
85 stars 8 forks source link

Guard for valid email address #57

Closed johanvergeer closed 5 years ago

johanvergeer commented 5 years ago

I just started using your library, and so far it is looking great. And at this point I'm missing something that can be used in a couple of my projects.

Has anyone thought of adding a clause like string.MustBeValidEmail(); . This seems to be something that can be very generic and useful for this library. If you would like this to be added I'm willing to make a contribution.

feO2x commented 5 years ago

Hey @johanvergeer, thanks for your kind words. Sure, let's add this assertion to Light.GuardClauses. Some thinks we should consider:

You're welcome to implement this assertion and send me a pull request.

feO2x commented 5 years ago

BTW: as a workaround, there is already a string.MustMatch assertion that you can use while this feature isn't released.

johanvergeer commented 5 years ago

@feO2x Thanks for your fast response. I do have an issue though.

It seems the project supports a lot of frameworks and I have resolved most of them by now. The only one left is .NETFramework,Version=v3.5,Profile=CompactFramework. The plain .NETFramework,Version=v3.5 is already resolved.

Do you know how I can resolve that last one?

feO2x commented 5 years ago

@johanvergeer I suggest you create a TargetFrameworks.props file as described in the building and contributing page in the wiki. Thus you do not have to resolve the dependencies that are not installed on your dev machine.

Please take a look at the rest of the wiki, too. If you have any further questions, please feel free to contact me.

feO2x commented 5 years ago

Implemented in 38d506d63b9f8c8807833e9af6b31d8264bd4d2e

feO2x commented 5 years ago

Hey @johanvergeer, before I'll publish your new to NuGet, I wanted to take a closer look to the email regular expression that you wrote:

new Regex(
    @"^[\w!#$%&'*+\-/=?\^_`{|}~]+(\.[\w!#$%&'*+\-/=?\^_`{|}~]+)*@((((\w+\-?)+\.)+[a-zA-Z]{2,4})|(([0-9]{1,3}\.){3}[0-9]{1,3}))$",
    RegexOptions.CultureInvariant | RegexOptions.ECMAScript)

I have several questions:

  1. Did you write the expression yourself or did you (partially) copy it from another source? You linked (https://emailregex.com/)[https://emailregex.com/] but the regular expressions on this site for .NET or C# are not identical to the one you wrote.
  2. Can you tell if this regular expression works in all cases supported by the official RFC 2822 specification, especially section 3.4.1 where email addresses (addr-ref) are defined?
  3. Is your regular expression the simplest one we can use? As shown in the benchmark picture of your pull request, the performance is not that great. My overall goal is to provide fast assertions that only take a few nanoseconds and with IsEmailAddress we already are in the range of microseconds. I know email validation is not easy, but if we could at least arrive in the range of several hundred nanoseconds, that would be great.
johanvergeer commented 5 years ago

Hi @feO2x,

To answer your questions:

I did use an existing regex pattern to start with, but not from https://emailregex.com/, since that one gave a lot of false positives. The one I used to start of was from https://www.rhyous.com/2010/06/15/csharp-email-regular-expression/. This one also didn't pass all tests so I made some changes.

The only ones that won't pass at this moment are:

For the test cases I used this list from MSDN.

It might be that I missed some that I'm not aware of.

Regarding performance. This would require some more research. I do know some regex, but I'm not a master in it.

I also have one more question for you. I tried to use TargetFrameworks.props as you described in the Wiki, but VS is still showing errers because of the missing dependencies. Is there something else I might be missing?

Also a suggestion. Would you like to add a .gitignore to the project?

feO2x commented 5 years ago

Hey @johanvergeer,

  1. I will remove the link to https://emailregex.com/, and I will mention that https://www.rhyous.com/2010/06/15/csharp-email-regular-expression/ was used as a basis.
  2. I will check if compiling the regex will result in a faster execution time.
  3. There already is a .gitignore file: gitignore in Repo
  4. I'm not sure why you cannot load Light.GuardClauses.csproj with a custom TargetFrameworks.props file. Could you show me the specific error messages (and create a new issue for that, together with information about your specific VS version and the installed .NET / .NET Core frameworks)?
feO2x commented 5 years ago

Sorry that it took so long, but version 6.2 is out now. You can fetch your contribution on NuGet or in the source file.

Thanks again for your contribution!

johanvergeer commented 5 years ago

Hi @feO2x,

Just out of curiosity. Were you able to improve the performance of the regex?

feO2x commented 5 years ago

Unfortunately not. I think the key here is to replace the regular expression with plain code, but this would take some time.