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

Use diff thresholds for domain, top-level domain #84

Merged
merged 1 commit into from Aug 7, 2014

Conversation

jdpopkin
Copy link
Contributor

@jdpopkin jdpopkin commented Aug 6, 2014

Use a higher threshold when checking for an exact match with an entire
domain. This is useful because otherwise we have a problem where
corrections can "chain" - test@yahooo.cmo leads to a suggestion of
test@yahooo.com, which leads to a suggestion of yahoo.com.

If the user was already corrected once, they shouldn't have to be
corrected again - especially since they're using the answer we just
gave them.

@weilu
Copy link
Contributor

weilu commented Aug 7, 2014

Why wouldn't you want test@yahooo.cmo to be corrected to test@yahoo.com, which is more correct than test@yahooo.com?

@jdpopkin
Copy link
Contributor Author

jdpopkin commented Aug 7, 2014

With this fix, test@yahooo.cmo is now corrected to test@yahoo.com. Prior to this fix, test@yahooo.cmo would be corrected to test@yahooo.com, which would be corrected to test@yahoo.com. (Try it out on Kicksend's sign up page - http://kicksend.com/join - it's pretty weird.)

I guess the test doesn't make that as clear as it should be - it just tests that only one suggestion is made. I might change that.

Use a higher threshold when checking for an exact match with an entire
domain. This is useful because otherwise we have a problem where
corrections can "chain" - test@yahooo.cmo leads to a suggestion of
test@yahooo.com, which leads to a suggestion of yahoo.com.
@jdpopkin
Copy link
Contributor Author

jdpopkin commented Aug 7, 2014

Updated with a somewhat clearer spec and variable name.

Mailcheck currently suggests test@yahooo.com for test@yahooo.cmo. Then it would suggest test@yahoo.com after you accepted the test@yahooo.com suggestion. This PR fixes that, making it suggest test@yahoo.com for test@yahooo.cmo.

@weilu
Copy link
Contributor

weilu commented Aug 7, 2014

Thanks!

weilu added a commit that referenced this pull request Aug 7, 2014
Use diff thresholds for domain, top-level domain
@weilu weilu merged commit 52ed1d4 into mailcheck:master Aug 7, 2014
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

2 participants