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

Add support for IP version 4 or 6 for isIPRange function #1594

Merged
merged 1 commit into from
Apr 17, 2021
Merged

Add support for IP version 4 or 6 for isIPRange function #1594

merged 1 commit into from
Apr 17, 2021

Conversation

neilime
Copy link
Contributor

@neilime neilime commented Feb 5, 2021

At the moment, isIPRange function only supports IP v4, this PR allows to choose v4 or v6 or both (like isIP function)

Checklist

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

@codecov
Copy link

codecov bot commented Feb 5, 2021

Codecov Report

Merging #1594 (f7562ab) into master (deb1d1e) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1594   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          100       100           
  Lines         1807      1820   +13     
=========================================
+ Hits          1807      1820   +13     
Impacted Files Coverage Δ
src/lib/isIPRange.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 deb1d1e...f7562ab. Read the comment docs.

@fredleger
Copy link

Nice one ! Need this too

@renanmontebelo
Copy link
Contributor

Would this be a duplicate of #1568? Yours looks like it's a more complete solution though.
Maybe a discussion should be setup and close one of the PRs in favor of the other.

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.

Sorry for the late review. Thank you @neilime for your contribution! I really like your approach and the fact that you used the same logic behind isIP.
Can you please remove validator.js and validator.min.js (We stopped tracking those files in the library) and discard the unrelated changes in src/lib/isMobilePhone ?
Thank you 🎉

@tux-tn tux-tn added needs-more-review 🧹 needs-update For PRs that need to be updated before landing labels Mar 8, 2021
@neilime
Copy link
Contributor Author

neilime commented Mar 9, 2021

@tux-tn I have rebased with master, let me know if everything looks good to you

@neilime neilime requested a review from tux-tn March 9, 2021 17:38
@tux-tn
Copy link
Member

tux-tn commented Mar 9, 2021

@neilime LGTM ! thank you for making the necessary changes 🎉

@tux-tn tux-tn added ready-to-land For PRs that are reviewed and ready to be landed and removed 🧹 needs-update For PRs that need to be updated before landing needs-more-review labels Mar 9, 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.

Thanks for your contrib! 🎉

@profnandaa profnandaa merged commit 5d6db63 into validatorjs:master Apr 17, 2021
@neilime neilime deleted the feat/is-ip-range-version-4-6 branch April 17, 2021 15:30
@tux-tn tux-tn mentioned this pull request Oct 2, 2021
3 tasks
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.

5 participants