Skip to content

Conversation

nibra
Copy link
Contributor

@nibra nibra commented Apr 1, 2021

Pull Request for Issue #32

Summary of Changes

  • Add tests verifying the bug. These tests reveal several other issues, see Drone report.

ATTN: This PR builds on PR #31

Testing Instructions

Run the tests (or just check Drone report).

Documentation Changes Required

No

@nibra nibra changed the title Refactoring/ip helper Refactoring IpHelper Apr 1, 2021
@nibra nibra marked this pull request as draft April 1, 2021 12:24
@nibra nibra linked an issue Apr 1, 2021 that may be closed by this pull request
@nibra nibra marked this pull request as ready for review April 1, 2021 21:52
@nibra nibra self-assigned this Apr 2, 2021
@nibra nibra requested a review from a team April 2, 2021 01:16
@Llewellynvdm
Copy link

I have been reading over the changes here and it all looks brilliant. Did not yet pull and test them manually.

I have one question you are making changes to a number of public functions very dramatically... what back-porting, or warning of change, do we have for those who have used those. As they are public, and therefore it does have a measure of promise, right?

@Llewellynvdm
Copy link

Llewellynvdm commented Apr 2, 2021

There are more... but I think you get my question. Not that I don't agree the changes are valid. Just indeed going to break stuff. Besides the fact that the whole class has mutated...

@nibra
Copy link
Contributor Author

nibra commented Apr 2, 2021

Those name changes do no harm, as method names are not case sensitive. The readability, though, is better with uppercase IP.

@nibra
Copy link
Contributor Author

nibra commented Apr 2, 2021

The refactoring (and the added tests) changed the CRAP index from 8190 down to 53... Some parts of the original code were never able to run; it was trying to treat a netmask as an integer for IPv6 ranges...

@Llewellynvdm
Copy link

Yea, I get that... just trying to catch the guys like me who may have started using these methods directly...

@nibra
Copy link
Contributor Author

nibra commented Apr 2, 2021

API was restricted to the public methods; the class originally was final, so protected members were not accessible from outside.

nibra added a commit that referenced this pull request Aug 14, 2022
@rdeutz
Copy link
Contributor

rdeutz commented Jul 6, 2025

@nibra do you want to update the PR to 3.x or 4.x? If not we are going to close it.

@Hackwar Hackwar changed the base branch from 1.x-dev to 4.x-dev July 9, 2025 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IpHelper does not detect invalid IP addresses
5 participants