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 blacklist option to isEmail() #1377

Closed
wants to merge 1 commit into from

Conversation

Hzbbaum
Copy link

@Hzbbaum Hzbbaum commented Jul 12, 2020

pull request for isssue 654
Added an option in isEmail() to for blacklisted_chars, that recieves a string and checks if the name part
of the mail contains it, and returns false if it does.
added some tests and updated the readme.

Checklist

  • PR contains only changes related; no stray files, etc.
  • README updated (where applicable)
  • Tests written (where applicable)

Copy link
Member

@profnandaa profnandaa left a comment

Choose a reason for hiding this comment

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

Thanks for your PR!

  1. could you please provide some justification for your change?
  2. remove all the unrelated changes/files.

@Hzbbaum
Copy link
Author

Hzbbaum commented Jul 14, 2020

thank you for your response. I would like some quick pointers, if possible.

  1. as for justification, this PR is meant to solve issue 654 which was tagged help wanted. please advise what further justification is required.
  2. my apologies, but this is my first PR. The important changes are to readme, src/lib/isEmail.js, and test/validators.js. All the other changes occur when i run the test command as part of the compile. should I submit only the 3 changes?

@profnandaa
Copy link
Member

@Hzbbaum -- got it, thanks!

Sure, as for the unrelated changes auto-generated, you can remove them from the PR, you can only stage the related changes.

@profnandaa profnandaa added the 🧹 needs-update For PRs that need to be updated before landing label Jul 28, 2020
@rubiin
Copy link
Member

rubiin commented Sep 28, 2020

@profnandaa this pr seems stale, more than two months and still no reply from author. If you like i could pull up the fork and do necessary chnages so that it can be merged

@profnandaa
Copy link
Member

Sure, please do a new one. Thanks!

@profnandaa profnandaa closed this Sep 28, 2020
@rubiin
Copy link
Member

rubiin commented Sep 29, 2020

sure, gimme a day or two , i wll clean things up

@rubiin rubiin mentioned this pull request Sep 30, 2020
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧹 needs-update For PRs that need to be updated before landing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants