my attempt to resolve #17 #19
Conversation
|
@skeggse I don't mean to rush, but any news on whether this is ready for merging? I went over the changes, and they look straight forward. |
|
Hey, sorry for not commenting. Here's why I haven't merged: This PR does not handle full unicode code points, and doesn't back its changes up with references to the relevant RFCs. In my spare time, I've been reading through said RFCs, but there are quite a few that specifically related to unicode support in the email ecosystem. |
|
That is definitely fair! I have not read the RFCs, but I can imagine since they are detailed and long, it will take a while to implement them properly. I am not good at interpreting RFCs, but I can definitely try and help if needed! |
|
@navroopsingh @skeggse - is this PR still in progress? We are needing to support international characters in our production code, and have Joi/isemail integrated throughout our codebase. If this PR has staled, we will have someone on our team work with you to see if we can't get RFC 6530 compliance worked in :) |
|
[EDIT] sorry, didn't realize I was still commenting on the PR instead of the issue. Moving the comment that was here to the issue instead. |
|
@skeggse - I left a couple of comments on #18 regarding the |
|
Just to clarify: as far as I can tell, this PR does pass the test cases in #18, disregarding the DNS issues in those cases. |
|
It does break lab rules. It also uses |
|
I see your comments in #17, and those aren't covered here, but to play devil's advocate the code already uses |
|
Yes, the code does. It doesn't support the full code space of Unicode, and
was written before codePointAt. As the task is to add Unicode support, it
should fix existing code to support Unicode, no?
…On Mon, Feb 6, 2017, 7:01 PM Wes Tyler ***@***.***> wrote:
I see your comments in #17 <#17>,
and those aren't covered here, but to play devil's advocate the code
already uses charCodeAt in several places. This PR didn't introduce that
usage :)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#19 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABSVf8N4P4A9b5He4JPaLYOb3SGZUZOdks5rZ7RtgaJpZM4IZgez>
.
|
|
Haha yessir :)
…On Monday, February 6, 2017, Eli Skeggs ***@***.***> wrote:
Yes, the code does. It doesn't support the full code space of Unicode, and
was written before codePointAt. As the task is to add Unicode support, it
should fix existing code to support Unicode, no?
On Mon, Feb 6, 2017, 7:01 PM Wes Tyler ***@***.***
***@***.***');>> wrote:
> I see your comments in #17 <#17
>,
> and those aren't covered here, but to play devil's advocate the code
> already uses charCodeAt in several places. This PR didn't introduce that
> usage :)
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#19 (comment)>, or
mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/
ABSVf8N4P4A9b5He4JPaLYOb3SGZUZOdks5rZ7RtgaJpZM4IZgez>
> .
>
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#19 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ALl4O-NAxjHPwwYa0z9EPegEY4oYu17qks5rZ7WHgaJpZM4IZgez>
.
|
|
More than a year later, this functionality has landed in |
my attempt to resolve Support unicode email addresses #17 to cover unicode email address with characters uptil Integer 65535 as returned by charCodeAt( ) , in localpart and domain.
added 5 unicode test cases at the end.
updated 2 old test cases cases from
to