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(isIP): allow usage of options object #2089

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

pixelbucket-dev
Copy link

@pixelbucket-dev pixelbucket-dev commented Oct 25, 2022

This PR implements steps 1 and 2 of #1874 for isIP and builds upon #2075.

This PR extracts tests for isIP into a separate test file ⇾ test/validators/isIP.test.js (inspired by #1793).

I have also added three more tests to guard against other possible values for version.

Checklist

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

@codecov
Copy link

codecov bot commented Oct 25, 2022

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (0cace52) compared to base (54d330c).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #2089   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          105       105           
  Lines         2324      2324           
  Branches       586       587    +1     
=========================================
  Hits          2324      2324           
Impacted Files Coverage Δ
src/lib/isIP.js 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@braaar
Copy link
Contributor

braaar commented Oct 26, 2022

if we rebase/merge this with #2091, the diff will be much cleaner and easier to review

@pixelbucket-dev
Copy link
Author

pixelbucket-dev commented Oct 26, 2022

if we rebase/merge this with #2091, the diff will be much cleaner and easier to review

I've merged #2091 into this branch. But the diff won't be improved until #2091 is merged.

@braaar
Copy link
Contributor

braaar commented Oct 26, 2022

if we rebase/merge this with #2091, the diff will be much cleaner and easier to review

I've merged #2091 into this branch. But the diff won't be improved until #2091 is merged.

Ah, of course. And I suppose it's possible to set 2091 as the base branch, either.

@WikiRik
Copy link
Member

WikiRik commented Oct 26, 2022

Ah, of course. And I suppose it's possible to set 2091 as the base branch, either.

It is possible, but then it would be a PR in my fork and not this repo directly. So we'll have to deal with this workaround for now

@WikiRik WikiRik added the 🧹 needs-update For PRs that need to be updated before landing label Jan 30, 2023
@pixelbucket-dev
Copy link
Author

I think this should be good to go :).

Copy link
Member

@WikiRik WikiRik left a comment

Choose a reason for hiding this comment

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

Can you undo the changes to the isAlpha tests?

@pixelbucket-dev
Copy link
Author

Can you undo the changes to the isAlpha tests?

Sorry, what exactly do you mean? I cannot see anything related to isAlpha in the diff view.

@WikiRik
Copy link
Member

WikiRik commented Feb 5, 2023

In the big test file you removed too much for this PR causing the codecov check to fail

@pixelbucket-dev
Copy link
Author

In the big test file you removed too much for this PR causing the codecov check to fail

Done :).

@pixelbucket-dev
Copy link
Author

Can this be re-reviewed?

@rubiin rubiin requested a review from WikiRik July 9, 2024 09:45
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