Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upadded verify email and extract login example #249
Conversation
This comment has been minimized.
This comment has been minimized.
|
Hi @Ophirr33 thanks for all the submissions! Unfortunately I'll not be able to follow up with anything resembling review for the next two weeks due to holidays |
This comment has been minimized.
This comment has been minimized.
|
@budziq No worries! |
budziq
requested changes
Jul 16, 2017
| Validates that an email address is formatted correctly, and extracts everything | ||
| before the @ symbol. | ||
|
|
||
| ```rust,no_run |
This comment has been minimized.
This comment has been minimized.
|
|
||
| ```rust,no_run | ||
| extern crate regex; | ||
| use regex::Regex; |
This comment has been minimized.
This comment has been minimized.
| use regex::Regex; | ||
| // A simple email validator and extractor. Validates that something has an @ | ||
| // symbol, followed by something that looks like a domain. |
This comment has been minimized.
This comment has been minimized.
budziq
Jul 16, 2017
Collaborator
I would suggest to move such descriptions to top with rest of the text. Also I would try to keep the comments as brief as possible (or even get rid of them in favour of textual description above) as we do not try to teach regular expressions here just show off the crate and snippet.
| // A simple email validator and extractor. Validates that something has an @ | ||
| // symbol, followed by something that looks like a domain. | ||
| // Returns what came before the @ symbol if everything looks valid. | ||
| fn extract_login(input: &str) -> Option<String> { |
This comment has been minimized.
This comment has been minimized.
budziq
Jul 16, 2017
Collaborator
- Option<Cow> return type would be nicer .
- using lazy_static seams to be the common idiom when compiling the regex
https://github.com/brson/rust-cookbook/pull/248 is a fine example of both
| fn main() { | ||
| assert_eq!( | ||
| extract_login(r"I❤email@example.com"), | ||
| Some(r"I❤email".to_owned()) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Addressed the comments, and I also put the example behind the lazy static examples. |
| }; | ||
| } | ||
| RE.captures(input) | ||
| .and_then(|cap| cap.get(1).map(|login| Cow::from(login.as_str()))) |
This comment has been minimized.
This comment has been minimized.
| lazy_static! { | ||
| static ref RE: Regex = { | ||
| let mut pattern = r"^([^@\s]+)@".to_owned(); | ||
| pattern += r"([[:word:]]+\.)*[[:word:]]+$"; |
This comment has been minimized.
This comment has been minimized.
budziq
Jul 22, 2017
Collaborator
no need to construct the regex in such way please use multiline regex (whitespace insensitive) definition syntax directly within Regex::new() like in
https://brson.github.io/rust-cookbook/basics.html#run-an-external-command-and-process-stdout
any (super brief) comments would go into the regex body comments (starting with #)
| use std::borrow::Cow; | ||
| // Validates that something has an @ symbol, followed by something that | ||
| // looks like a domain. Returns what came before the @ symbol, if everything |
This comment has been minimized.
This comment has been minimized.
budziq
Jul 22, 2017
Collaborator
i would pare the comments down as much as possible or even remove them (the example should be self explanatory as much as possible.)
| // Validates that something has an @ symbol, followed by something that | ||
| // looks like a domain. Returns what came before the @ symbol, if everything | ||
| // looks valid. Prevents regex recompilation using lazy_static! | ||
| fn extract_login(input: &str) -> Option<Cow<str>> { |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Hi @Ophirr33. I'm back from holidays so I should be able to give some more hands on help here. Do you need any assistance with this example? |
This comment has been minimized.
This comment has been minimized.
|
Deleted the comments and tried to use a named capture to make the code more self-explanatory |
This comment has been minimized.
This comment has been minimized.
|
thanks! merged |
Ophirr33 commentedJul 15, 2017
Fixes #240. Verifies that the text looks like an email address (anything that's not white space or an @, followed by an @, followed by period joined sequences of word characters).