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

Added support for hyphen(-) under email regex #543

Merged
merged 1 commit into from
Apr 11, 2023

Conversation

hiteshbedre
Copy link
Contributor

Current version of email regex don't have support for hyphen(-). I have updated the existing regex which has duplicate slash symbol twice in first square bracket, which is redundant.

Executed some test cases here: https://regex101.com/r/qiNqNc/1

@TommyLemon TommyLemon merged commit 026c7be into Tencent:master Apr 11, 2023
@TommyLemon
Copy link
Collaborator

thx for ur contribution! besides, i got a good tool for testing regular expressions because of ur mention above~

@TommyLemon
Copy link
Collaborator

TommyLemon commented Apr 11, 2023

btw, after i typed

tommy@qq.com
lemon-31@gmail.com

in the left input area, it shows that the 2 lines don't match the regular expression above.

then i modified the lines to:

tommy@XX\XXX\
lemon-31@XX\XXX\

it shows they matched.

is there anything wrong?

image

@hiteshbedre
Copy link
Contributor Author

email ending with ".com" is handled separately in code.
Here: https://github.com/Tencent/APIJSON/blob/master/APIJSONORM/src/main/java/apijson/StringUtil.java#L678

@TommyLemon
Copy link
Collaborator

TommyLemon commented Apr 12, 2023

yeah, that's a problem of the legacy code, and i think we can remove the if clause here:
https://github.com/Tencent/APIJSON/blob/master/APIJSONORM/src/main/java/apijson/StringUtil.java#L678-L680

plus, if (isNotEmpty(email, true) == false) can be simplified to if (isEmpty(email, true))
https://github.com/Tencent/APIJSON/blob/master/APIJSONORM/src/main/java/apijson/StringUtil.java#L673

what do u think?

@hiteshbedre
Copy link
Contributor Author

Absolutely, plus we can completely change the email regex to validate ".com" part within regex itself. Happy to do it.

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.

2 participants