Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7k
IPAddressField improvements#2747
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
rest_framework/fields.py Outdated
There was a problem hiding this comment.
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 theunpack_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 dataThere was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- the user choose to use 'both' because he/she needs to validate both ipv4 and ipv6.
- the use passes '192.168.0.1' as value, and the value is automatically 'unpacked'.
- the output of 192.168.0.1 is: 2002:c0a8:0001::c0a8:0001
are you sure this is what we want?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actually I got this wrong again (oh gosh... I really suck with this!). unpack_ipv4 does exactly the opposite: given an ip that is an ipv6 representation of an ipv4, unpack_ipv4 transforms back the ipv6 to ipv4. This is proven by these testshttps://github.com/django/django/blob/0ed7d155635da9f79d4dd67e4889087d3673c6da/tests/utils_tests/test_ipv6.py#L53
andreagrandi commentedApr 2, 2015
@tomchristie I apologize for the long delay in fixing this. It should now be ok to be merged. |
jpadilla commentedApr 26, 2015
LGTM. @tomchristie@xordoquy for when should we milestone this, 3.1.2 or 3.2.0? |
jpadilla commentedJun 2, 2015
Milestoned this as good to go for 3.2.0 |
lovelydinosaur commentedJun 2, 2015
No problem from me with this going into a minor release instead, if that's better with@xordoquy. Either way okay. |
lovelydinosaur commentedJun 3, 2015
Pushing this forward to either 3.1.3 (or 3.1.4 if needed) |
IPAddressField improvementsxordoquy commentedJun 3, 2015
I'll update and merge this tonight. |
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/