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

Fixes to library to parse "some@email.com (comments) <some@email.com>" #46

Merged
merged 3 commits into from
Oct 24, 2019

Conversation

chesnokovilya
Copy link
Contributor

@chesnokovilya chesnokovilya commented Jul 11, 2019

There are a lot of mail addresses written in the form some@email.com (comments) <some@email.com>
Currently library does not parse them correctly. (it would output undefined when parsing this name)

This is causing some issues with pgp (pretty good privacy) realisations:
openpgpjs/openpgpjs#918

In this pull request I fix this behaviour to correctly parse displayName with @. Secondly comments are correctly parsed and outputted as concatenated string.

@chesnokovilya chesnokovilya changed the title Fixes to library to make it compatible with existing email implementations Fixes to library to parse "some@email.com (comments) <some@email.com>" Jul 11, 2019
@tomholub
Copy link

@jackbearheart gentle ping. Thanks!

@twiss
Copy link

twiss commented Jul 18, 2019

This library aims to implement RFC 5322, while OpenPGP clients are more lenient than that. I've created a fork using these commits at https://github.com/openpgpjs/email-addresses, but it might not be appropriate to include them in the original, as these User IDs are not in fact RFC 5322 compliant.

@jackbearheart
Copy link
Owner

Thanks for the PR! I appreciate you folks wanting to contribute this upstream if possible.

twiss is correct in my intent here. This PR as is isn't right for this library because it would change the default behavior to be non-rfc compliant. What we could do is add an option acceptAtSignInDisplayName (or something more concise) that enables this. There are some other options like this floating around.

@chesnokovilya
Copy link
Contributor Author

chesnokovilya commented Jul 19, 2019

ok, I will look at the option path.
What do you think regarding concatenating comments @jackbearheart? Currently library make comments on email unaccessible.

@jackbearheart
Copy link
Owner

@chesnokovilya concatenating comments like you did seems reasonable

@chesnokovilya
Copy link
Contributor Author

chesnokovilya commented Aug 11, 2019

I have updated PR. Now this feature is behind option atInDisplayName which is false by default.
@jackbearheart

@jackbearheart
Copy link
Owner

@chesnokovilya Great, thank you! I haven't had the chance to merge and release this yet, but from a brief look I think it is good. I'll do this when I have a moment.

@chesnokovilya
Copy link
Contributor Author

Great

@JoeNyland
Copy link

@jackbearheart Any chance this can be merged and a new release out out soon, please?

@jackbearheart
Copy link
Owner

Yes, I'll do that now. Apologies for the delay...

@jackbearheart jackbearheart merged commit 4d9ab8f into jackbearheart:master Oct 24, 2019
@jackbearheart
Copy link
Owner

I published this in version 3.1.0. I made some minor changes to preserve backwards compatibility. Thank you!

@JoeNyland
Copy link

Awesome, thank you!

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.

None yet

5 participants