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

Revise ruby valid logic to one line #79

Merged
merged 1 commit into from
Oct 5, 2016

Conversation

dav1a1223
Copy link
Contributor

Dear @FGRibreau ,
When I use this handy gem and read the source code,
I found the logic of valid? and valid_email? a little complex.
So I took a look at .py and revise the logic to one line.
Hoping that make it easier to understand :)
I'm a beginner to contribute open source, please give me your advice. :)
Thx a lot!!

return false unless valid_email?(email)

!blacklisted?(email)
return valid_email?(email) && !blacklisted?(email)
Copy link

@JulienBreux JulienBreux Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

valid_email?(email) && !blacklisted?(email)

return false if email.nil?

email =~ EMAIL_REGEX
return !email.nil? && !!(email =~ EMAIL_REGEX)
Copy link

@JulienBreux JulienBreux Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!email.nil? && !!(email =~ EMAIL_REGEX)

return false unless valid_email?(email)

!blacklisted?(email)
return valid_email?(email) && !blacklisted?(email)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

valid_email?(email) && !blacklisted?(email)

return false if email.nil?

email =~ EMAIL_REGEX
return !email.nil? && !!(email =~ EMAIL_REGEX)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!email.nil? && !!(email =~ EMAIL_REGEX)

@FGRibreau
Copy link
Owner

Hello @dav1a1223,

as @JulienBreux said, just remove the return and I will accept this PR :)

@dav1a1223
Copy link
Contributor Author

OK, it's done. @FGRibreau @JulienBreux thanks a lot !!

@FGRibreau FGRibreau merged commit 73d2538 into FGRibreau:master Oct 5, 2016
@FGRibreau
Copy link
Owner

@dav1a1223 nice work, released in v3.0.16 :)

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.

3 participants