-
-
Notifications
You must be signed in to change notification settings - Fork 30k
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 and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bpo-18802: ipaddress documentation changes #6083
Conversation
Doc/library/ipaddress.rst
Outdated
@@ -91,7 +91,8 @@ Address objects | |||
The :class:`IPv4Address` and :class:`IPv6Address` objects share a lot of common | |||
attributes. Some attributes that are only meaningful for IPv6 addresses are | |||
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. |
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.
:term:`hashable`
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.
Done
Doc/library/ipaddress.rst
Outdated
@@ -368,6 +369,7 @@ All attributes implemented by address objects are implemented by network | |||
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. |
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.
Ditto
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.
Done
Doc/library/ipaddress.rst
Outdated
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. |
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 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.
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.
"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).
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.
Sorry, Jon, I pushed my changes while you were leaving this comment. I will make another commit once the proper wording is worked out.
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.
@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.
Doc/library/ipaddress.rst
Outdated
@@ -722,6 +727,8 @@ Network objects can act as containers of addresses. Some examples:: | |||
Interface objects | |||
----------------- | |||
|
|||
Interface objects are hashable, so they can be used as keys in dictionaries. |
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.
:term:`hashable`
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.
Done
Doc/library/ipaddress.rst
Outdated
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 |
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.
Not true any more. Changed in commit 7bd8d3e. IP address takes part in comparison and it's no longer strange.
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've removed that and added a :versionchanged:
for this new comparison.
Lib/test/test_ipaddress.py
Outdated
@@ -476,6 +476,12 @@ class InterfaceTestCase_v4(BaseTestCase, NetmaskTestMixin_v4): | |||
class NetworkTestCase_v4(BaseTestCase, NetmaskTestMixin_v4): | |||
factory = ipaddress.IPv4Network | |||
|
|||
def test_no_mask(self): |
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 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')
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.
Moved, thanks.
@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. |
Doc/library/ipaddress.rst
Outdated
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`. |
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.
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.
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.
Thank you. I think I've captured it correctly now.
@jonfoster Any thoughts on my previous reply?
|
@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:
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. |
Thanks @csabella for the PR, and @zhangyangyu for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7. |
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 . |
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>
Original patch by Jon Foster and Berker Peksag. (cherry picked from commit 5609b78) Co-authored-by: Cheryl Sabella <cheryl.sabella@gmail.com>
GH-6167 is a backport of this pull request to the 3.6 branch. |
Original patch by Jon Foster and Berker Peksag.
Original patch by Jon Foster and Berker Peksag.
https://bugs.python.org/issue18802