-
Notifications
You must be signed in to change notification settings - Fork 5k
Add MailAddress.TryCreate api #1052
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
} | ||
|
||
parsedData = (displayName, info.User, info.Host, displayNameEncoding); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opted for tuple because is used only inside this class
@@ -1,218 +1,181 @@ | |||
// Licensed to the .NET Foundation under one or more agreements. | |||
// The .NET Foundation licenses this file to you under the MIT license. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reorganized data to allow reuse and equality with ctors tests.
{ | ||
try | ||
{ | ||
return input.Normalize(Text.NormalizationForm.FormC); | ||
normalizedString = input.Normalize(Text.NormalizationForm.FormC); | ||
return true; | ||
} | ||
catch (ArgumentException e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only place where we need to rely on exception handling.
/azp list |
Commenter does not have sufficient privileges for PR 1052 in repo dotnet/runtime |
/azp list |
Commenter does not have sufficient privileges for PR 1052 in repo dotnet/runtime |
@ViktorHofer Shouldn't the external-ci group have azp permissions? |
just asked #718 (comment) |
/azp list |
Commenter does not have sufficient privileges for PR 1052 in repo dotnet/runtime |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments, but generally LGTM.
@dotnet/ncl, can someone please review? |
src/libraries/System.Net.Mail/src/System/Net/Mail/MailAddress.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/System/Net/Mail/DomainLiteralReader.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Mail/src/System/Net/Mail/MailAddress.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Mail/src/System/Net/Mail/MailAddress.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Mail/src/System/Net/Mail/MailAddress.cs
Outdated
Show resolved
Hide resolved
Co-Authored-By: Stephen Toub <[email protected]>
Co-Authored-By: Stephen Toub <[email protected]>
Co-Authored-By: Stephen Toub <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for the PR!
closes #907
cc: @scalablecory @davidsh