bpo-18802: ipaddress documentation changes by csabella · Pull Request #6083 · python/cpython

csabella · 2018-03-11T22:55:16Z

Original patch by Jon Foster and Berker Peksag.

https://bugs.python.org/issue18802

zhangyangyu · 2018-03-12T01:29:39Z

 also implemented by :class:`IPv4Address` objects, in order to make it easier to
-write code that handles both IP versions correctly.
+write code that handles both IP versions correctly.  Address objects are
+hashable, so they can be used as keys in dictionaries.


:term:`hashable`

zhangyangyu · 2018-03-12T01:30:33Z

 objects as well.  In addition, network objects implement additional attributes.
 All of these are common between :class:`IPv4Network` and :class:`IPv6Network`,
 so to avoid duplication they are only documented for :class:`IPv4Network`.
+Network objects are hashable, so they can be used as keys in dictionaries.


zhangyangyu · 2018-03-12T01:37:01Z

-      interpreted as a *net mask* if it starts with a non-zero field, or as
-      a *host mask* if it starts with a zero field.  If no mask is provided,
-      it's considered to be ``/32``.
+      interpreted as a *net mask* if possible, or as a *host mask* otherwise.


I don't like the change here. It's confusing. What is if possible mean? With the below note, the original doc is fine. Or refer to the comment of IPv4Network.__init__:

If the mask (portion after the / in the argument) is given in dotted quad form, it is treated as a netmask if it starts with a non-zero field (e.g. /255.0.0.0 == /8) and as a hostmask if it starts with a zero field (e.g. 0.255.255.255 == /8), with the single exception of an all-zero mask which is treated as a netmask == /0. If no mask is given, a default of /32 is used.

"starts with a zero field" is wrong. 128.0.0.0 is* accepted as the netmask for a /1 network, 127.255.255.255 is* accepted as the hostmask for a /1 network, neither starts with a zero field.

How about: "interpreted as a net mask if it is a valid net mask, or interpreted as a host mask otherwise."?

(* Or they were when I originally wrote the patch, I haven't looked to see if the code has changed since).

Sorry, Jon, I pushed my changes while you were leaving this comment. I will make another commit once the proper wording is worked out.

@jonfoster Thanks for your reply! But I always understand the non-zero field means the binary zero not octet zero. So 128.0.0.0 is starting with non-zero field and should be netmask. And 127.255.255.255 is starting with zero field and should be hostmask.

zhangyangyu · 2018-03-12T01:39:12Z

 Interface objects
 -----------------

+Interface objects are hashable, so they can be used as keys in dictionaries.


:term:`hashable`

zhangyangyu · 2018-03-12T02:10:25Z

+address objects with the same IP version can be compared, and the address
+objects will always sort before the interface objects.  Two interface objects
+are compared by comparing their networks, using the same rules as
+:class:`IPv4Network` or :class:`IPv6Network`.  The IP address plays no part in


Not true any more. Changed in commit 7bd8d3e. IP address takes part in comparison and it's no longer strange.

I've removed that and added a :versionchanged: for this new comparison.

zhangyangyu · 2018-03-12T02:23:49Z

 class NetworkTestCase_v4(BaseTestCase, NetmaskTestMixin_v4):
    factory = ipaddress.IPv4Network

+    def test_no_mask(self):


I suggest move this equal test to NetmaskTestMixin_v4.test_valid_netmask. It could also be applied to ip interface objects then.

self.assertEqual(str(self.factory('1.2.3.4')), '1.2.3.4/32')

Moved, thanks.

csabella · 2018-03-12T21:45:38Z

@zhangyangyu Thanks for the review. I've made the requested changes. The original patch creator, @jonfoster, also left a comment that still needs to be addressed once the best wording is determined.

zhangyangyu · 2018-03-13T02:42:50Z

+address objects with the same IP version can be compared, and the address
+objects will always sort before the interface objects.  Two interface objects
+are compared by comparing their networks and IP addresses, using the same
+rules as :class:`IPv4Network` or :class:`IPv6Network`.


Network objects are compared by network address and then netmask. So saying the rules are same is not suitable. I suggest something like "Two interface objects are compared by their networks. If they are same, IP addresses are compared." And "versionchanged" is not needed, the commit is treated as a bugfix and backported to 3.6. So in all bugfix releases, the behaviour is consistent.

Thank you. I think I've captured it correctly now.

zhangyangyu · 2018-03-19T02:36:18Z

@jonfoster Any thoughts on my previous reply?

Thanks for your reply! But I always understand the non-zero field means the binary zero not octet zero. So 128.0.0.0 is starting with non-zero field and should be netmask. And 127.255.255.255 is starting with zero field and should be hostmask.

jonfoster · 2018-03-19T20:35:48Z

@zhangyangyu my understanding of "field" was "octet", but I can't find a good definition of "field" in this context. I think it would be clearer if you change "field" to "bit". How about:

the mask is interpreted as a net mask if the most significant bit (of the 32-bit address) is set, or as a host mask if the most significant bit is zero.

If you disagree, I think it's OK to merge this branch as-is - it is definitely a major improvement over the status quo, and I don't want to stand in the way of landing it.

miss-islington · 2018-03-21T00:09:17Z

Thanks @csabella for the PR, and @zhangyangyu for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7.
🐍🍒⛏🤖

zhangyangyu · 2018-03-21T00:10:37Z

I leave it same as the comment in code for now since "significant bit" needs a conversion from dot form to integer. Thanks @csabella and @jonfoster .

bedevere-bot · 2018-03-21T00:10:43Z

GH-6166 is a backport of this pull request to the 3.7 branch.

Original patch by Jon Foster and Berker Peksag. (cherry picked from commit 5609b78) Co-authored-by: Cheryl Sabella <cheryl.sabella@gmail.com>

bedevere-bot · 2018-03-21T00:11:27Z

GH-6167 is a backport of this pull request to the 3.6 branch.

Original patch by Jon Foster and Berker Peksag. (cherry picked from commit 5609b78) Co-authored-by: Cheryl Sabella <cheryl.sabella@gmail.com>

Original patch by Jon Foster and Berker Peksag.

csabella added 2 commits March 11, 2018 18:51

bpo-18802: ipaddress documentation changes

da04b31

Add blurb

ef85b99

the-knights-who-say-ni added the CLA signed label Mar 11, 2018

bedevere-bot added the awaiting review label Mar 11, 2018

zhangyangyu reviewed Mar 12, 2018

View reviewed changes

zhangyangyu added needs backport to 3.6 labels Mar 12, 2018

zhangyangyu reviewed Mar 12, 2018

View reviewed changes

Review changes

d47f516

zhangyangyu reviewed Mar 13, 2018

View reviewed changes

Clarify text for interface comparison

2ecbb07

update

e0a100e

zhangyangyu merged commit 5609b78 into python:master Mar 21, 2018

bedevere-bot removed the awaiting review label Mar 21, 2018

bedevere-bot removed the needs backport to 3.7 label Mar 21, 2018

bedevere-bot removed the needs backport to 3.6 label Mar 21, 2018

miss-islington added a commit that referenced this pull request Mar 21, 2018

bpo-18802: Add more details to ipaddress documentation (GH-6083)

a323eee

Original patch by Jon Foster and Berker Peksag. (cherry picked from commit 5609b78) Co-authored-by: Cheryl Sabella <cheryl.sabella@gmail.com>

miss-islington added a commit that referenced this pull request Mar 21, 2018

bpo-18802: Add more details to ipaddress documentation (GH-6083)

481cbe8

Original patch by Jon Foster and Berker Peksag. (cherry picked from commit 5609b78) Co-authored-by: Cheryl Sabella <cheryl.sabella@gmail.com>

csabella deleted the bpo18802 branch March 21, 2018 12:50

jo2y pushed a commit to jo2y/cpython that referenced this pull request Mar 23, 2018

bpo-18802: Add more details to ipaddress documentation (pythonGH-6083)

f08ad32

Original patch by Jon Foster and Berker Peksag.

Uh oh!

Conversation

csabella commented Mar 11, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhangyangyu Mar 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhangyangyu Mar 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonfoster Mar 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhangyangyu Mar 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csabella commented Mar 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhangyangyu commented Mar 19, 2018

Uh oh!

jonfoster commented Mar 19, 2018

Uh oh!

miss-islington commented Mar 21, 2018

Uh oh!

zhangyangyu commented Mar 21, 2018

Uh oh!

bedevere-bot commented Mar 21, 2018

Uh oh!

bedevere-bot commented Mar 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

csabella commented Mar 11, 2018 •

edited by bedevere-bot

Loading

zhangyangyu Mar 12, 2018 •

edited

Loading

zhangyangyu Mar 12, 2018 •

edited

Loading

jonfoster Mar 12, 2018 •

edited

Loading

zhangyangyu Mar 12, 2018 •

edited

Loading