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

Allow comma in display name #52

Merged
merged 7 commits into from
Feb 26, 2021
Merged

Allow comma in display name #52

merged 7 commits into from
Feb 26, 2021

Conversation

osm
Copy link
Contributor

@osm osm commented Feb 15, 2021

This is a similar PR as I made earlier #47, but this time it allows , in the display name.

The feature was added to the email-addresses library with this PR: jackbearheart/email-addresses#54

@msimerson
Copy link
Member

msimerson commented Feb 22, 2021

I'm not strongly opposed to allowing this, but after perusing RFC 5322 quickly, and considering the security aspects of the Display Name, I'm not convinced it should default to enabled. I think the feature should default to off.

@osm
Copy link
Contributor Author

osm commented Feb 25, 2021

That sounds reasonable.

I decided to add an options object after the startAt parameter that lets the user toggle comma in display name parsing. It would have been nicer to also include the startAt parameter within the options object but that would mean a breaking change. So I think this is probably the best way to do it.

@msimerson
Copy link
Member

Switching to an object for passing in arguments is the right approach. The called function can check the argument type to maintain backwards compatibility.

@osm
Copy link
Contributor Author

osm commented Feb 25, 2021

Yeah it's nicer, with the latest commit I've changed it to allow for an object to be passed instead and at the same time also accept anything else and use that as the startAt parameter to keep backwards compatibility.

index.js Outdated Show resolved Hide resolved
@msimerson
Copy link
Member

And while you're at it, please update Changes.md and bump the version number in package.json.

@msimerson msimerson merged commit c6e2ab7 into haraka:master Feb 26, 2021
@msimerson msimerson mentioned this pull request Feb 23, 2024
msimerson added a commit that referenced this pull request Feb 23, 2024
- feat: option to allow comma in display name #52
- dep(email-addresses): bump from 4.0.0 to 5.0.0 #58
- chore: replace a couple regex with slice (perf & sec) #63
- test: a few more tests to boost coverage #63
- test: drop node 10, add node 16 #61
- ci: restore GH workflow for PRs #57
- ci: add dependabot.yml #55
- doc: add inline documentation for parse #60
- doc(Changes): make PR #s into links #54
- doc(README): add result of console.logs #56
- doc(README): add links to RFC 2822, 5322 #53
- doc(README): enabled syntax highlighting with code fences #51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants