-
Notifications
You must be signed in to change notification settings - Fork 43
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
Implement allowUnicode option (defaults to true) #160
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, thanks!
It's now been a long time since I originally wrote this code, and it's become less and less familiar to me. I suspect we'll need to add checks to other places such as here, given that the outer loop isn't the only place we access code point data.
lib/index.js
Outdated
if (typeof options.allowUnicode !== 'undefined') { | ||
allowUnicode = Boolean(options.allowUnicode); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simpler: const allowUnicode = options.allowUnicode === undefined || !!options.allowUnicode;
lib/index.js
Outdated
token = String.fromCodePoint(email.codePointAt(i)); | ||
const codePoint = email.codePointAt(i); | ||
if (!allowUnicode && codePoint > 0x7f) { | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ignores the errorLevel
option - at the very least, it should output an appropriate code when diagnose
is true
.
This demonstrates the imperfections of the errorLevel system as implemented, because the user might set allowUnicode
to true
, but set the errorLevel
higher than the error code corresponding to disallowed unicode.
I guess we could also just note that this is the behavior for allowUnicode
, and that errorLevel
isn't really compatible with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to map it to an errorLevel
value here: c3430a0
Let me know what you think.
Feels a bit weird, as predicted by @skeggse skeggse#160 (comment)
@skeggse, thanks for taking a look! I think I covered all your points now :) |
Thanks! I think the error code you selected is about right; what do you think about making that error level specified by Isemail.validate('a@b', {
// Allow unicode
unicodeErrorLevel: 0
// Disallow unicode
errorLevel: 20,
unicodeErrorLevel: 25
}); I'm not super attached to either, and the PR as-is could ship otherwise - just wanted some feedback on whether you think this would be a better approach. |
@skeggse, hmm, I'm not sure I have an opinion about that. Trying hard to put myself in the shoes of someone using the library in that way where various degrees of invalidity on a linear scale is required. Personally I think I'd rather specify exactly what I want to allow with individual options, but due to the amount of error cases I also see why that doesn't scale here. Alternatively I guess I'd pick or make a more opinionated library that seemed reasonable so that I wouldn't need to dive into the details :) |
That would be fairly easy to do, I guess. For consistency with |
I think we just take this as-is, and resolve the remaining issue in #162. |
As discussed on #156
This might be rather naive as I haven't looked at the full grammar. Let me know if there's anything it should cover :)