Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
lovelydinosaur merged 8 commits intoencode:masterfromandreagrandi:feature/ipaddress-fix
Jun 4, 2015
Merged

IPAddressField improvements#2747

lovelydinosaur merged 8 commits intoencode:masterfromandreagrandi:feature/ipaddress-fix
Jun 4, 2015

Conversation

@andreagrandi
Copy link
Contributor

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/

Copy link
Contributor

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 data

Copy link
ContributorAuthor

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.

Copy link
ContributorAuthor

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?

Copy link
ContributorAuthor

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
Copy link
ContributorAuthor

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

@jpadilla
Copy link
Contributor

LGTM.

@tomchristie@xordoquy for when should we milestone this, 3.1.2 or 3.2.0?

@jpadilla
Copy link
Contributor

Milestoned this as good to go for 3.2.0

@jpadillajpadilla changed the titleIPAddressField and tests imporvementsIPAddressField and tests improvementsJun 2, 2015
@lovelydinosaur
Copy link
Contributor

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

@lovelydinosaur
Copy link
Contributor

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.

@xordoquyxordoquy changed the titleIPAddressField and tests improvementsIPAddressField improvementsJun 3, 2015
@xordoquy
Copy link
Contributor

I'll update and merge this tonight.

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

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

3.1.3 Release

Development

Successfully merging this pull request may close these issues.

5 participants

@andreagrandi@jpadilla@lovelydinosaur@xordoquy@Ins1ne

[8]ページ先頭

©2009-2025 Movatter.jp