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

feat(isEmail): add host_blacklist option #1641

Merged
merged 3 commits into from
Oct 2, 2021

Conversation

fedeci
Copy link
Contributor

@fedeci fedeci commented Apr 10, 2021

We add an option to allow the user to exclude some specific domains from isEmail validation.
Fixes #1639

Checklist

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

@codecov
Copy link

codecov bot commented Apr 10, 2021

Codecov Report

Merging #1641 (ac4896c) into master (05ceb18) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1641   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          100       100           
  Lines         1847      1849    +2     
=========================================
+ Hits          1847      1849    +2     
Impacted Files Coverage Δ
src/lib/isEmail.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05ceb18...ac4896c. Read the comment docs.

Copy link
Member

@tux-tn tux-tn left a comment

Choose a reason for hiding this comment

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

A little suggestion, we have a similar option in isURL called host_whitelist and host_blacklist that accepts hosts and regexes. What do you think about matching that behaviour to have consistency between validators options?

@fedeci
Copy link
Contributor Author

fedeci commented Apr 16, 2021

It makes sense but I would keep the denylist instead of blacklist to use an inclusive language at least in new options.

I have nothing against blacklist and whitelist, but someone may be offended by such terms.

@tux-tn
Copy link
Member

tux-tn commented Apr 16, 2021

It makes sense but I would keep the denylist instead of blacklist to use an inclusive language at least in new options.

I have nothing against blacklist and whitelist, but someone may be offended by such terms.

Project maintainer views on this matter have already been shared here #1522

@fedeci fedeci changed the title feat(isEmail): add domain_denylist option feat(isEmail): add host_blacklist option Apr 17, 2021
@fedeci
Copy link
Contributor Author

fedeci commented Apr 17, 2021

It's always a good time to change your mind 🤷‍♂️

@fedeci
Copy link
Contributor Author

fedeci commented Oct 2, 2021

Ping?

Copy link
Member

@tux-tn tux-tn left a comment

Choose a reason for hiding this comment

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

LGTM 🎉
Sorry for the long review delay, i missed the notification about new commits in this PR !

@tux-tn tux-tn added the ready-to-land For PRs that are reviewed and ready to be landed label Oct 2, 2021
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.

LGTM, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-land For PRs that are reviewed and ready to be landed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow business only email address
3 participants