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.

Sign up for GitHub

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

Jump to bottom

feat(isEmail): add host_blacklist option #1641

Merged
merged 3 commits into from
Oct 2, 2021
Merged

feat(isEmail): add host_blacklist option #1641

merged 3 commits into from
Oct 2, 2021

Conversation

fedeci
Copy link
Contributor

@fedeci fedeci commented Apr 10, 2021 •
edited
Loading

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 •
edited
Loading

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.

tux-tn
tux-tn reviewed Apr 16, 2021
View reviewed changes
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 •
edited
Loading

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 •
edited
Loading

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?

tux-tn
tux-tn approved these changes Oct 2, 2021
View reviewed changes
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
profnandaa
profnandaa approved these changes Oct 2, 2021
View reviewed changes
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!

@profnandaa profnandaa merged commit 04b73ad into validatorjs:master Oct 2, 2021
@fedeci fedeci deleted the email-domain-denylist branch October 2, 2021 23:02
@profnandaa profnandaa mentioned this pull request Oct 31, 2021
Closed
@WikiRik WikiRik mentioned this pull request Jan 26, 2022
Closed
@abdulrahman-khankan abdulrahman-khankan mentioned this pull request Nov 22, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tux-tn tux-tn tux-tn approved these changes

@profnandaa profnandaa profnandaa approved these changes

Assignees
No one assigned
Labels
ready-to-land For PRs that are reviewed and ready to be landed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow business only email address
3 participants
@fedeci @tux-tn @profnandaa