-
Notifications
You must be signed in to change notification settings - Fork 56
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
Directive to Split Email Address into Account and Domain #26
Conversation
String[] parts = ((String) object).split("@"); | ||
if (parts.length > 1) { | ||
record.add(column + ".account", parts[0]); | ||
record.add(column + ".domain", StringUtils.join(ArrayUtils.subarray(parts, 1, parts.length), "@")); |
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 think this is actually backwards. you want to use the last "part" as the domain (since it can't contain additional @ symbols) and join all the first parts as the account. http://stackoverflow.com/a/36297137
List<Record> records = Arrays.asList( | ||
new Record("email", "root@cask.co"), | ||
new Record("email", "joltie.xxx@gmail.com"), | ||
new Record("email", "joltie_xxx@hotmail.com") |
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.
can you add a test for an email that looks like this: a."b@c".d@e.f
after you fix the parsing above?
Assert.assertNotNull(records.get(0).getValue("email.domain")); | ||
Assert.assertEquals("root.hotmail.com", records.get(1).getValue("email.account")); | ||
} | ||
} |
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.
newline
…rangler into feature/directive-email-split
} | ||
if (object instanceof String) { | ||
String emailAddress = (String) object; | ||
int nameIdx = emailAddress.lastIndexOf("<"); // Joltie, Root <joltie.root@yahoo.com> |
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.
should you have an option to store the "display name" along with account and domain? could also be a future enhancement
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.
It will be a separate directive to extract that.
Assert.assertNotNull(records.get(0).getValue("email.domain")); | ||
Assert.assertNull(records.get(1).getValue("email.account")); | ||
} | ||
} |
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.
newline
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.
done!
minor comments. once those are addressed, LGTM |
Directive checklist