IPAddressField improvements by andreagrandi · Pull Request #2747 · encode/django-rest-framework (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation12 Commits8 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

andreagrandi

This PR is an extension of this original one #2618

What I did was reviewing the code and adding a couple of test cases to the IPAddressField.

This code was produced as part of the Django Sprint hosted by Potato in London: http://djangosprint.london/

tomchristie

try:
return clean_ipv6_address(data, self.unpack_ipv4)
except DjangoValidationError:
self.fail('invalid', value=data)

Choose a reason for hiding this comment

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

Looking good. I think we arrived at a slight tweak to this, that involved us getting rid of the unpack_ipv4 flag and just always having that as True for 'both'. Something like this?...

if self.protocol in ('both', 'ipv6'):
    unpack_ipv4 = (self.protocol == 'both')
    return clean_ipv6_address(data)
return data

Choose a reason for hiding this comment

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

Hi Tom,

I'm away from my computer for few days so I will be able to reply this more properly at the end of the week. Basically I noticed that ipv4_unpack was needed anyway by the method that returns validator, that's why I left it. I need to check if tests still pass always defaulting it to True when is both. It doesn't have to be always True, nor always false.

Choose a reason for hiding this comment

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

I'm now back from holidays. I've given a look to the methods. The problem we may have forcing the unpack_ipv4 in case of 'both' is the following:

are you sure this is what we want?

Choose a reason for hiding this comment

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

@andreagrandi

@andreagrandi

@andreagrandi

@tomchristie I apologize for the long delay in fixing this. It should now be ok to be merged.

@jpadilla

@jpadilla

Milestoned this as good to go for 3.2.0

@jpadilla jpadilla changed the titleIPAddressField and tests imporvements IPAddressField and tests improvements

Jun 2, 2015

@tomchristie

No problem from me with this going into a minor release instead, if that's better with @xordoquy. Either way okay.

@tomchristie

Pushing this forward to either 3.1.3 (or 3.1.4 if needed)
Don't see any need to wait for 3.2.0 for it.
Needs updating to master, but otherwise looks good to go.

@xordoquy xordoquy changed the titleIPAddressField and tests improvements IPAddressField improvements

Jun 3, 2015

@xordoquy

I'll update and merge this tonight.

This was referenced

Mar 9, 2017

This was referenced

Oct 6, 2017

This was referenced

Oct 16, 2017

This was referenced

Nov 6, 2017

This was referenced

Nov 14, 2017

This was referenced

Dec 10, 2017