Skip to content

Conversation

@Ins1ne
Copy link
Contributor

@Ins1ne Ins1ne commented Feb 28, 2015

for this #1853

@jpadilla jpadilla added this to the 3.1.0 Release milestone Feb 28, 2015
@lovelydinosaur
Copy link
Contributor

Looks good! I could do with some time to review the implementation fully, simply to make sure if there are any clean-ups we can make or not, but everything else seems great.

Choose a reason for hiding this comment

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

This check is unnecessary. '' will always just pass though to the final return.

@lovelydinosaur
Copy link
Contributor

Going to drop the 3.1 marker on this so that I make sure to take the time to review properly. (We're about to release tomorrow, so...)

Copy link
Contributor

Choose a reason for hiding this comment

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

I may be wrong, but this line looks like we assume the user will always use 'both' protocol, but what happen if we can't unpack the ipv4 address to an ipv6 one? In this case ':' won't be present inside the ip string and the whole check will fail or be skipped.

Is this situation already handled by the use of ip_address_validators method? Should we add a particular test case that handle the situation where the user pass (protocol='ipv4', unpack_ipv4=True) ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds right!

@jpadilla
Copy link
Contributor

jpadilla commented Jun 2, 2015

Closing this in favor of #2747

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants