Skip to content

Conversation

linted
Copy link
Contributor

@linted linted commented May 15, 2018

I noticed that the way validation of ip addresses were being handled only supports ipv4, so I changed it to a built in method of ip validation.

@codingo codingo self-assigned this May 15, 2018
@codingo codingo added the bug label May 15, 2018
@codingo
Copy link
Owner

codingo commented May 15, 2018

Nice catch. Looks like this is still failing some of our test cases, do you mind correcting and then I can test?

@linted
Copy link
Contributor Author

linted commented May 15, 2018

Yeah I am looking at it now. I just want to make sure of something first, is this supposed to run on python2 or python3? the install instructions say to use python2 but the travis-cli is running it off of python3. the ipaddress module in python2 needs unicode strings which require code that I think is breaking in the python3 run.

@codingo
Copy link
Owner

codingo commented May 15, 2018

Definitely python3. I've just checked the installation instructions and can't see a reference to Python 2.x. Could you let me know where you saw this?

@codingo codingo changed the title Changed to a more robust ip validation which also takes care of ipv6 Update IP validation to support IPv6 May 15, 2018
@linted
Copy link
Contributor Author

linted commented May 15, 2018

Oh, sorry! When I read python setup.py install in the readme's installation guide, I assumed that meant to run everything with python 2. It might be helpful to change that to python3 setup.py install to help people copy/pasting on systems with both versions installed.

I have an idea why this might not be running on py3, but I won't be able to get to it till tomorrow. I'll let you know if my fix works then.

@codingo
Copy link
Owner

codingo commented May 15, 2018

Not a problem, no rush! I'll raise a different issue to revisit the setup instructions.

@linted
Copy link
Contributor Author

linted commented May 15, 2018

I realized that I actually forgot to add the import statement for the ip_address function. It was hidden by the try except block around the function. I feel dumb lol

@linted
Copy link
Contributor Author

linted commented May 16, 2018

@codingo hey I tried my hand at updating the tests to check for ipv6 support. I just added a ipv6 address to the prefix and suffix tests. Is that the way you would like the test, or should I create a separate test for the ipv4 and ipv6 addresses?

@codingo
Copy link
Owner

codingo commented May 17, 2018

I think separate tests would be the best approach for this. Could you add to this PR?

@linted
Copy link
Contributor Author

linted commented May 17, 2018

Cool, I made the ipv6 checks different functions. looks like everything is working.

@codingo codingo merged commit 10150da into codingo:master May 17, 2018
@codingo
Copy link
Owner

codingo commented May 17, 2018

Excellent, all looks good. Thank-you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants