[feature] RFC2822 parsing by TracyGJG · Pull Request #3708 · moment/moment

TracyGJG · 2017-01-08T08:19:08Z

I have corrected the issue with the failing test but I still have a test I am unable to perform with out some advice. The test case 'RFC2822 datetime with mismatching Day (week v date)' confirms the day is consistent when given as both a day of week and day of month. This is a stipulation of the RFC so needs to be correct but I cannot achieve this without using a JS Date object, unless the core team have an alternative solution.

Work in Progress - Do not Merge, Advice required. Issue #2530

…te literal with a complex string.

ichernev · 2017-02-04T10:45:35Z

+        ' PDT': ' -0700',
+        ' PST': ' -0800'
+    };
+    var rfc2822Military = 'YXWVUTSRQPONZABCDEFGHIKLM';


can you please remove rfc2822 from the names of things inside the method named configFromRFC2822 :)

ichernev · 2017-02-04T10:47:23Z

@TracyGJG that looks pretty clean to me. Can you please summarize what is the current state, are there any places where this differs from the spec ((a) we parse it but its not spec or (b) we don't parse it but its spec), we can drop most comments and it should be good to go.

TracyGJG · 2017-02-04T12:43:26Z

@ichernev, as requested I have 'Removed rfc2822 prefix from internal variables.' in my latest commit. To answer your question regarding compliance with the spec. To be fully compliant (afaik) we have to ensure the any day given in the sting as a weekday is consistent with the date given. I have not been able to perform this check and I have disabled the test. [TODO: src/lib/create/from-string.js line 163, src/test/create.js line 448]. I had tried creating a moment from the date only elements and extract the weekday but this caused a conflict - any suggestions.

TracyGJG · 2017-02-13T21:22:50Z

@ichernev, @maggiepint and @mj1856, I finally have a fully compliant (AFAIK) implementation of RFC2822. In order to validate the weekday against the date I have had to use the native JavaScript Date object. I have left in the code a TODO to replace with an independent approach. However, as I have not managed to discover an alternative after several months, this approach will have to suffice.

ichernev · 2017-02-14T10:00:53Z

@TracyGJG the proper way to do the check about weekday matching is using the validation/flag system, take a look at src/lib/create/from-string-and-array.js and more notably the modification of getParsingFlags() (its just lazily returning an object). src/lib/create/parsing-flags.js has all of them -- you can add one like weekdayMismatch which defaults to false, but is set to true in your RFC parsing function. If someone wants to parse RFC and calls your function and you parse it and figure out the weekday is mismatching, doesn't mean you abandon the parse, quite the opposite -- you set the flag and call it done (that is -- you parsed correctly and produced an invalid moment).

Whether you first produce a moment object, then figure out its bad, or you first figure out its bad (by using a Date) ... there is no difference for the end user, and we can safely switch one to the other without breaking compatibility in the future.

…ation.

This reverts commit a9cdeef.

TracyGJG · 2017-02-14T18:07:34Z

@ichernev, thanks for the steer. I have added the weekdayMismatch flag as suggested, defaulted to false and set to true should the check fails. I have retained setting the _isValid flag and exit (return).

if (match[1].substr(0,3) !== momentDay) {
    getParsingFlags(config).weekdayMismatch = true;
    config._isValid = false;
    return;
}

Is the above code appropriate or should it be simplified?

ichernev

Almost ready to ship ;-)

ichernev · 2017-03-05T22:05:22Z

+        (\:(?:60|[0-5]\d))?
+        (\s(?:UT|GMT|[ECMP][SD]T|[A-IK-Z]|(?:[+-](?:1[012]|0\d)[03]0)))
+    $/;
+*/


Can you please shorten this comment, remove the expanded regex, if possible just link to the source (RFC).

ichernev · 2017-03-05T22:05:37Z

+        ' PDT': ' -0700',
+        ' PST': ' -0800'
+    };
+    var Military = 'YXWVUTSRQPONZABCDEFGHIKLM';


this should be lowercase military.

ichernev · 2017-03-05T22:07:44Z

+                return;
+            }
+        }
+        getParsingFlags(config).rfc2822 = true;


I think we need to set this flag after a successful parse at the bottom, otherwise it might get out-of-sync

Now that I rechecked -- the ISO parsing has the same issue. We need to set the flag only after a fully successful parse.

ichernev · 2017-03-05T22:10:38Z

    configFromISO(config);
    if (config._isValid === false) {
        delete config._isValid;
-        hooks.createFromInputFallback(config);


this code should be linear but is instead nested. I'd return in case isValid becomes true, so more parsers can be added without increasing indentation.

ichernev · 2017-03-05T22:11:21Z

+test('non RFC 2822 strings', function (assert) {
+    assert.ok(!moment('Tue. 01 Nov 2016 01:23:45 GMT', moment.RFC_2822, true).isValid(),
+        'RFC2822 datetime with all options but invalid day delimiter');
+    assert.ok(!moment('Mon, 01 Nov 2016 01:23:45 GMT', moment.RFC_2822, true).isValid(),


also check if the proper parsing flag is set.

ichernev · 2017-03-05T22:11:51Z

    moment.relativeTimeThreshold('M', 11);
 });

+/* TGJG Disabled


I think you need to reenable this code.

westy92 · 2017-03-06T06:40:41Z

        parsedDateParts : [],
-        meridiem        : null
+        meridiem        : null,
+        rfc2822			: false,


Test case re-enabled.

Spacing of the rfc2822 parse flag syntax aligned.

configFromString() made linear, Military variable made lowercase and RegExp comment replaced with tools.ietf reference.

Semi-colons

Trailing white-space removed

Issue with configFromString()

TracyGJG · 2017-03-06T20:37:51Z

@ichernev, all comments addressed including the syntax issue raised by @westy92.
All tests pass and no issues remain. Now ready to merge.

Made the military time zone case insensitive in line with specification.

ichernev · 2017-03-12T00:14:48Z

Merged in 7589bb8

[feature] RFC2822 parsing

TracyGJG added 5 commits November 4, 2016 22:29

Work in Progress - Do Not Merge

59abb49

Timezone/offset processing added.

b1ceb7e

Near-candidate solution

8110754

Revised RFC2822 regular expressions

4e913ea

Test case 'RFC2822 datetime with CFWSs' revised to replace the templa…

bb955ca

…te literal with a complex string.

TracyGJG mentioned this pull request Jan 8, 2017

RFC2822 Parsing implementation - Near-candidate solution #3579

Closed

TracyGJG changed the title ~~Work in Progress - Do Not Merge (updated 8th Jan 2017)~~ RFC2822 Parsing implementation - Near-candidate solution, tests working Jan 9, 2017

ichernev reviewed Feb 4, 2017

View reviewed changes

Removed rfc2822 prefix from internal variables.

39f4df6

TracyGJG added 5 commits February 4, 2017 13:08

Trial validation of weekday

247fc3e

Further trial

a84506f

Style correction

e203fbb

Revoked weekday check and test

c3e874d

Removed import of moment library.

37500aa

maggiepint mentioned this pull request Feb 9, 2017

Trouble parsing US Timezone abbreviations moment/moment-timezone#445

Closed

TracyGJG added 4 commits February 13, 2017 20:19

Trial day-of-week validation using a vanilla Date object.

289de6c

Retest rfc2822

c8d4257

Correction: Style of comment!

30ddde4

Removed the breaking comments in the test case.

bb9cf02

TracyGJG added 4 commits February 14, 2017 17:38

Parsing flag weekdayMismatch added and applied to the RFC parse valid…

7e23f8f

…ation.

Style correction.

bde9e7b

Further style correction.

a9cdeef

Revert "Further style correction."

8e75e62

This reverts commit a9cdeef.

ichernev reviewed Mar 5, 2017

View reviewed changes

westy92 reviewed Mar 6, 2017

View reviewed changes

TracyGJG and others added 11 commits March 6, 2017 12:15

Update relative_time.js

d73864c

Test case re-enabled.

Update parsing-flags.js

759435d

Spacing of the rfc2822 parse flag syntax aligned.

Update from-string.js

988480e

configFromString() made linear, Military variable made lowercase and RegExp comment replaced with tools.ietf reference.

Update from-string.js

0297345

Semi-colons

Update from-string.js

75da615

Trailing white-space removed

Update from-string.js

5c615cb

Issue with configFromString()

rfc2822 ParsingFlags moved to the bottom of the parsing code.

a510013

Positive and Negative RFC2822 testCases streamlined.

48e67f0

Missing semi-colons added.

cc44152

Checking rfc2822 parsingFlags

f443dfd

Changed parsingFlags().rfc2822 from a function call

496ec07

TracyGJG added 5 commits March 6, 2017 21:47

Enhanced positive test cases to include a variety of time zone/offsets.

4672f1b

Made the military time zone case insensitive in line with specification.

Correction to military case insensitive time zone

fee7e59

Testcases revised to track down issue.

5a6da4c

Zulu conversion correction.

2bcb711

Further correction

e059191

TracyGJG changed the title ~~RFC2822 Parsing implementation - Near-candidate solution, tests working~~ RFC2822 Parsing implementation - Ready for merge Mar 8, 2017

ichernev added this to the 2.18.0 milestone Mar 12, 2017

ichernev added the Pending Next Release label Mar 12, 2017

ichernev added a commit that referenced this pull request Mar 12, 2017

Merge pull request #3708 from TracyGJG:develop

7589bb8

[feature] RFC2822 parsing

ichernev changed the title ~~RFC2822 Parsing implementation - Ready for merge~~ [feature] RFC2822 parsing Mar 12, 2017

ichernev closed this Mar 12, 2017

icambron mentioned this pull request Mar 12, 2017

Updated the String parsing section to include reference to the RFC 2822 format moment/momentjs.com#399

Merged

mattjohnsonpint removed the Pending Next Release label Apr 12, 2017

Conversation

TracyGJG commented Jan 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ichernev commented Feb 4, 2017

Uh oh!

TracyGJG commented Feb 4, 2017

Uh oh!

TracyGJG commented Feb 13, 2017

Uh oh!

ichernev commented Feb 14, 2017

Uh oh!

TracyGJG commented Feb 14, 2017

Uh oh!

ichernev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TracyGJG commented Mar 6, 2017

Uh oh!

ichernev commented Mar 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

TracyGJG commented Jan 8, 2017 •

edited

Loading