-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Added is_valid_email_address.rs #783
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #783 +/- ##
==========================================
+ Coverage 95.33% 95.34% +0.01%
==========================================
Files 310 311 +1
Lines 22657 22821 +164
==========================================
+ Hits 21600 21759 +159
- Misses 1057 1062 +5 ☔ View full report in Codecov by Sentry. |
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.
The definition of the valid email address is slightly more involved, cf. https://en.wikipedia.org/wiki/Email_address#Examples
in particular, "john..doe"@example.org
is perfectly fine.
This can be merged only if all of the edge cases are covered.
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 rewrote some of the code and now passes all of the tests on https://en.wikipedia.org/wiki/Email_address#Examples
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.
Please make sure that tests cover all of the lines.
I handled all the things that you requested. |
Please make sure to update your branch (cf. #789) - otherwise the CI will fail. |
0c45ece
to
2d4da60
Compare
I think I fixed all the issues and made the IP checking better. |
382360a
to
284439b
Compare
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.
Please consider using more consistent naming: sometime the valid
part is in the middle and sometimes at the end. Perhaps it is not needed at all?
05040a6
to
88d09e3
Compare
c443bff
to
89f10ef
Compare
fn is_valid_local_part(local_part: &str) -> bool { | ||
if local_part.len() > MAX_LOCAL_PART_LENGTH as usize { | ||
return false; | ||
} | ||
|
||
if !is_valid_quotes(local_part) { | ||
return false; | ||
} | ||
|
||
if !contains_legal_local_characters(local_part) { | ||
return false; | ||
} | ||
|
||
if has_bad_dots(local_part) { | ||
return false; | ||
} | ||
|
||
true | ||
} |
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.
What do you think about having a similar approach as in is_valid_domain
? I.e. if the local_part
starts and ends "
. then a function is_valid_local_quoted_part
and is_valid_local_unquoted_part
?
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 don't think this would work as not all email addresses are made up of only a single string, for example a."b".c
is valid although it does not begin and end with quotes. In addition due to the fact that the contents of all strings are removed at the start of the local part checking, I believe that it would make the code less readable.
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.
a."b".c
is unquoted. It contains a "
character, therefore it is not valid (cf. wiki).
This pull request has been automatically marked as abandoned because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Please ping one of the maintainers once you commit the changes requested or make improvements on the code. If this is not the case and you need some help, feel free to ask for help in our Gitter channel. Thank you for your contributions! |
Changes
Type of change
Checklist:
cargo clippy --all -- -D warnings
just before my last commit and fixed any issue that was found.cargo fmt
just before my last commit.cargo test
just before my last commit and all tests passed.mod.rs
file within its own folder, and in any parent folder(s).DIRECTORY.md
with the correct link.COUNTRIBUTING.md
and my code follows its guidelines.