Skip to content
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

Add parentheses when missing for time zones #70

Merged
merged 1 commit into from
Jul 20, 2017
Merged

Add parentheses when missing for time zones #70

merged 1 commit into from
Jul 20, 2017

Conversation

veverkap
Copy link
Contributor

@veverkap veverkap commented Apr 4, 2017

Changes proposed in this pull request

In our usage of this library, we are seeing some timestamps in the format

03 Apr 2017 12:30:55 GMT

where the timezone does not include parentheses. This is a quick fix to add parentheses so that this will match the pattern on line 57

@bcardarella
Copy link
Member

Is 03 Apr 2017 12:30:55 GMT a valid ISO date format?

@veverkap
Copy link
Contributor Author

I didn't think that it was, but in our corpus, it shows up and the parser does choke on it. RFC 2822 doesn't require parentheses from my reading.

https://tools.ietf.org/html/rfc2822#page-14

@bcardarella
Copy link
Member

Let me explore this a bit more. I recognize that email formatting is so flexible that these deviations are likely to appear. I just want to be careful about turning the library into one that tries to cover all cases. I would prefer to allow opt-in from an app config or another method to support.

On the other hand, if this is part of the spec I'll merge :)

@bcardarella
Copy link
Member

So based upon the spec, this format is now obsolete:

https://tools.ietf.org/html/rfc2822#section-4.3

However, considering how permissive the spec is and how many permutations have been implemented there should be a way for us to provide support for deviations.

You may want to create your own renderer and parser derived from the RFC2822 modules that add this obsolete behavior for the time being. Thanks for the PR but considering it is implementing now obsolete behavior I'll close it out.

@bcardarella
Copy link
Member

Gave this some further thought, I am going to re-open. I think the parser should be able to consume the obsolete formats. This will likely turn into a larger issue as I would want to expand but may be able to accept this PR for this.

@bcardarella bcardarella reopened this Jul 20, 2017
@bcardarella bcardarella merged commit 165ae99 into DockYard:master Jul 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants