Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7k
add IPAddressField, update docs#2618
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
lovelydinosaur commentedMar 2, 2015
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. |
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.
This check is unnecessary.'' will always just pass though to the finalreturn.
lovelydinosaur commentedMar 4, 2015
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...) |
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 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) ?
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.
Sounds right!
jpadilla commentedJun 2, 2015
Closing this in favor of#2747 |
for this#1853