Support rfc6854 and parsing for From, Sender and Reply-To #31

baudehlo · 2017-06-11T17:36:29Z

Fixes #28

Updated docs and tests.

jackbearheart

These are my thoughts while running through this. If you see what I'm getting at, feel free to clean this up (oh also console.logs throughout). Otherwise I'll get to this within a couple weeks and clean it up myself.

jackbearheart · 2017-06-12T22:11:04Z

lib/email-addresses.js

-
+
+    function addrListGiveResult(ast) {
+        return _giveResult(ast, ['group', 'mailbox']);


Is it intentional that addrListGiveResult, fromGiveResult, and senderGiveResult have the same body of code? Is there a reason they may need to have different functionality? Either one should be different or there should be one function and no giveResMap. I do see that you wanted to hide the interface of _giveResult, but no need to have different function names.

No, I can clean that up now. It happened because they actually have different definitions, but it turns out what you want from the results for each is achieved the same way. I started out with different calls to _giveResults(). They ended up identical when I carefully examined the grammar and realized that everything boils down to address, which is mailbox / group anyway.

jackbearheart · 2017-06-12T22:26:58Z

lib/email-addresses.js

        ret.addresses = [];
        for (i = 0; i < addresses.length; i += 1) {
            addr = addresses[i];
+            if (addr.seen) continue;
+            addr.seen = true;
+            if (addr.name === 'group') {


It happens to work, because findAllNodes is first called on group and then on mailbox, but considering this loop by itself one might wonder if a node in a group could be processed before the group it is part of. If you called _giveResult(ast, ['mailbox', 'group']) somewhere else, you would get the wrong result, which is confusing.

I think it would be simpler to:

make this two functions (no recursion)

process all the 'group' nodes first (loop over findAllNodes('group', ast), then process non group nodes underneath that group, marking seen)

process all the non-group nodes second (loop over findAllNodes('mailbox', ast), skipping if "seen")

Both achieve things the same way, the "correct" way would be to know just how deep it's OK to go in the AST, IMHO. That way you could guarantee that looking for mailboxes under a group wouldn't happen.

jackbearheart · 2017-06-12T22:28:54Z

lib/email-addresses.js

-function parseOneAddressSimple(opts) {
-    var result;
-
+function _opts (opts, startAt) {


Merge this with handleOpts since it's not otherwise clear why there are two functions _opts and handleOpts.

Because _opts gives the defaults for the named functions. When called as a library (require('email-addresses')('foo@bar')) those defaults don't apply.

jackbearheart · 2017-06-12T22:32:03Z

lib/email-addresses.js

@@ -528,7 +528,21 @@ function parse5322(opts) {
            obsGroupList
        )());
    }
-
+
+    function fromSpec() {


// RFC 6854 2.1. Replacement of RFC 5322, Section 3.6.2. Originator Fields
// from = "From:" (mailbox-list / address-list) CRLF

jackbearheart · 2017-06-12T22:32:49Z

lib/email-addresses.js

+        )());
+    }
+
+    function senderSpec() {


// RFC 6854 2.1. Replacement of RFC 5322, Section 3.6.2. Originator Fields
// sender = "Sender:" (mailbox / address) CRLF

jackbearheart · 2017-06-12T22:33:19Z

lib/email-addresses.js

+            address
+        )());
+    }
+


Should we have a function for reply-to while we're here?

I figured we don't need it because reply-to is just address-list, so it's superfluous.

jackbearheart · 2017-06-12T22:37:09Z

test/email-addresses.js

+
+    result = fxn("Managing Partners:ben@example.com,carol@example.com;");
+    t.ok(result, "Parse group for Sender:");
+    t.equal(result.length, undefined, "Result is not an array");


This is a pretty loose test :)

Mea culpa, I know. I got tired :) Nobody likes writing tests. I'll improve.

baudehlo · 2017-06-13T00:10:33Z

Addressed the changes that were simple. Only remaining one might be giveResult() - let me know if you think it's important now it's contained to not be changeable.

whitespace add comments for originator fields, move to section 3.6.2 refactor giveResult refactor _opts process groups and mailboxes in one pass dfs instead of bfs so results are obviously in parse order quotes

jackbearheart · 2017-06-20T23:56:43Z

I had a bunch of extra time so I refactored a few things that were bugging me, beyond the original scope of the pr. I modified one of the test cases you wrote very slightly, but otherwise it is as you wrote the tests. The options read a bit more sanely now, so it should be easy to get the module function to do what you want or add more named functions. I also added some misc. "startAt" options that seemed like they might be interesting.

I'll merge this now -- ping me if I changed something you didn't like.

baudehlo · 2017-06-21T14:34:49Z

Looks great, thanks a lot!

When will you publish?

jackbearheart · 2017-06-21T17:37:41Z

@baudehlo Just published 3.0.0 on npm

Matt Sergeant and others added 9 commits June 9, 2017 18:41

Start of parsing rfc6854

adc468a

Works now. Needs more work on parseFrom etc

dc2fa5c

Implement results for From, Sender and ReplyTo

76228ce

get all the returned data right

b9679a3

First few tests

fff5ec8

Add documentation

074669f

Fix bullets

5efe65f

Fix group docs

2f3c7d7

Merge branch 'master' into support-rfc6854

66fdccf

jackbearheart reviewed Jun 12, 2017

View reviewed changes

Matt Sergeant added 2 commits June 12, 2017 20:08

Address PR comments

8f31f5a

Remove some whitespace changes

7437579

add comments, refactor giveResult and _opts, process nicely

ef29801

whitespace add comments for originator fields, move to section 3.6.2 refactor giveResult refactor _opts process groups and mailboxes in one pass dfs instead of bfs so results are obviously in parse order quotes

jackbearheart merged commit 2d6ca1e into jackbearheart:master Jun 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support rfc6854 and parsing for From, Sender and Reply-To #31

Support rfc6854 and parsing for From, Sender and Reply-To #31

baudehlo commented Jun 11, 2017

jackbearheart left a comment

jackbearheart Jun 12, 2017

baudehlo Jun 12, 2017

jackbearheart Jun 12, 2017

baudehlo Jun 12, 2017

jackbearheart Jun 12, 2017

baudehlo Jun 13, 2017

jackbearheart Jun 12, 2017

jackbearheart Jun 12, 2017

jackbearheart Jun 12, 2017

baudehlo Jun 12, 2017

jackbearheart Jun 12, 2017

baudehlo Jun 13, 2017

baudehlo commented Jun 13, 2017

jackbearheart commented Jun 20, 2017

baudehlo commented Jun 21, 2017

jackbearheart commented Jun 21, 2017



		function addrListGiveResult(ast) {
		return _giveResult(ast, ['group', 'mailbox']);

Support rfc6854 and parsing for From, Sender and Reply-To #31

Support rfc6854 and parsing for From, Sender and Reply-To #31

Conversation

baudehlo commented Jun 11, 2017

jackbearheart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baudehlo commented Jun 13, 2017

jackbearheart commented Jun 20, 2017

baudehlo commented Jun 21, 2017

jackbearheart commented Jun 21, 2017