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

[feature] RFC2822 parsing #3708

Closed
wants to merge 35 commits into
base: develop
from

Conversation

Projects
None yet
4 participants
@TracyGJG
Contributor

TracyGJG commented Jan 8, 2017

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

TracyGJG added some commits Nov 4, 2016

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

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

This comment has been minimized.

@ichernev

ichernev Feb 4, 2017

Contributor

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

@ichernev

This comment has been minimized.

Contributor

ichernev commented Feb 4, 2017

@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

This comment has been minimized.

Contributor

TracyGJG commented Feb 4, 2017

@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 added some commits Feb 4, 2017

@TracyGJG

This comment has been minimized.

Contributor

TracyGJG commented Feb 13, 2017

@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

This comment has been minimized.

Contributor

ichernev commented Feb 14, 2017

@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.

TracyGJG added some commits Feb 14, 2017

Revert "Further style correction."
This reverts commit a9cdeef.
@TracyGJG

This comment has been minimized.

Contributor

TracyGJG commented Feb 14, 2017

@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 ;-)

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

This comment has been minimized.

@ichernev

ichernev Mar 5, 2017

Contributor

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

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

This comment has been minimized.

@ichernev

ichernev Mar 5, 2017

Contributor

this should be lowercase military.

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

This comment has been minimized.

@ichernev

ichernev Mar 5, 2017

Contributor

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

This comment has been minimized.

@ichernev

ichernev Mar 5, 2017

Contributor

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

@@ -105,13 +213,19 @@ export function configFromString(config) {
configFromISO(config);
if (config._isValid === false) {
delete config._isValid;
hooks.createFromInputFallback(config);

This comment has been minimized.

@ichernev

ichernev Mar 5, 2017

Contributor

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.

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(),

This comment has been minimized.

@ichernev

ichernev Mar 5, 2017

Contributor

also check if the proper parsing flag is set.

@@ -139,6 +139,7 @@ test('custom thresholds', function (assert) {
moment.relativeTimeThreshold('M', 11);
});
/* TGJG Disabled

This comment has been minimized.

@ichernev

ichernev Mar 5, 2017

Contributor

I think you need to reenable this code.

@@ -12,7 +12,9 @@ function defaultParsingFlags() {
userInvalidated : false,
iso : false,
parsedDateParts : [],
meridiem : null
meridiem : null,
rfc2822 : false,

This comment has been minimized.

@westy92

TracyGJG added some commits Mar 6, 2017

Update relative_time.js
Test case re-enabled.
Update parsing-flags.js
Spacing of the rfc2822 parse flag syntax aligned.
Update from-string.js
configFromString() made linear, Military variable made lowercase and RegExp comment replaced with tools.ietf reference.
Update from-string.js
Semi-colons
Update from-string.js
Trailing white-space removed
Update from-string.js
Issue with configFromString()
@TracyGJG

This comment has been minimized.

Contributor

TracyGJG commented Mar 6, 2017

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

TracyGJG added some commits Mar 6, 2017

Enhanced positive test cases to include a variety of time zone/offsets.
Made the military time zone case insensitive in line with specification.

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

@ichernev ichernev added this to the 2.18.0 milestone Mar 12, 2017

@ichernev

This comment has been minimized.

Contributor

ichernev commented Mar 12, 2017

Merged in 7589bb8

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

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

@ichernev ichernev closed this Mar 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment