This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: ipadress compare_networks does not work according to documentation
Type: behavior Stage: resolved
Components: Documentation Versions: Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: docs@python Nosy List: Sanjay, docs@python, ncoghlan, pmoody, xiang.zhang
Priority: normal Keywords:

Created on 2017-03-27 07:23 by Sanjay, last changed 2022-04-11 14:58 by admin. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 842 closed Sanjay, 2017-03-27 07:25
PR 865 merged python-dev, 2017-03-28 05:49
Messages (9)
msg290566 - (view) Author: Sanjay (Sanjay) * Date: 2017-03-27 07:23
according to the docs compare_networks only checks the network address but the implementation is also taking the mask length into account. It returns '0' only if both the network address and the mask are equal but this can be done with just equality check ( ip1 == ip2 )

Example:
>>> ip1=ipaddress.ip_network("1.1.1.0/24")
>>> ip2=ipaddress.ip_network("1.1.1.0/25")
>>> ip1.compare_networks(ip2)
-1
>>> ip1 == ip2
False
>>> ip1.network_address
IPv4Address('1.1.1.0')
>>> ip2.network_address
IPv4Address('1.1.1.0')
>>> 

shouldn't we ignore the mask length ? I have tried it here:

https://github.com/s-sanjay/cpython/commit/942073c1ebd29891e047b5e784750c2b6f74494a
msg290569 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2017-03-27 08:55
In my mind this is the wanted behaviour. `compare_networks` means compare one network to another and a network should count its mask in. Your example would look rather weird if 1.1.1.0/24 == 1.1.1.0/25. Maybe make the doc from 'network addresses' to 'network bits' is easier to understand.
msg290571 - (view) Author: Sanjay (Sanjay) * Date: 2017-03-27 09:37
yes but compare_networks is not used to implement equality check. __eq__ correctly returns False when we do 1.1.1.0/24 == 1.1.1.0/25.
If compare_networks works exactly like __eq__ then it seems a bit redundant.
msg290572 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2017-03-27 09:44
Looking into it, this appears to be a holdover from the original ipaddr design where rather than being modelled as separate objects, host interfaces are modelled as denormalised network objects (i.e. a host address with a netmask).

So the stdlib equivalent of the original ipaddr compare_networks() operation would be:

    >>> ip_interface('192.0.2.1/31').network == ip_network('192.0.2.0/31')
True

As Sanjay noted, compare_networks() itself is fairly redundant given the stricter stdlib data model, since we don't allow the creation of denormalised network definitions in the first place:

```
>>> ip_network('192.0.2.1/31')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib64/python3.5/ipaddress.py", line 74, in ip_network
    return IPv4Network(address, strict)
  File "/usr/lib64/python3.5/ipaddress.py", line 1536, in __init__
    raise ValueError('%s has host bits set' % self)
ValueError: 192.0.2.1/31 has host bits set
```
msg290573 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2017-03-27 10:10
I still hold my opinion that the current behaviour is correct. You are comparing two networks and you should count the mask in. 

And notice that `neta == netb` is not totally equal to `neta.compare_networks(netb)`. The former can only results True or False but the later could result -1, 0 and 1. For example:

>>> IPv4Network('192.0.2.0/25') == IPv4Network('192.0.2.0/25')
True
>>> IPv4Network('192.0.2.0/25').compare_networks(IPv4Network('192.0.2.0/25'))
0
>>> 

They can't be used exchangably. `compare_networks` actually does the work of all <, >, ==.
msg290577 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2017-03-27 10:37
Right, I agree compare_networks() is behaving correctly according to its original design. However:

- it was designed for Python 2, where cmp() style idioms were still more common. Those idioms have largely been removed in Python 3 (no cmp() builtin, no __cmp__ magic method, no cmp argument to list.sort() and sorted())

- it was designed for the ipaddr data model, where network equality comparisons are more like ipaddress.ip_interface comparisons than they ipaddress.ip_network comparisons

Since there isn't any real maintenance burden in keeping the code around, I'm flipping this to be purely a documentation issue and suggesting that we:

- mark the method as deprecated in the docs, but *not* in the code (using the method form is just kind of pointless, not actively harmful)

- update the docs to say that it uses the same ordering and comparison algorithm as "<", "==", and ">" (the current confusion stems from the fact that the algorithms were different in ipaddr, but in the stdlib that other algorithm is the one used by interface objects, not network objects)
msg290579 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2017-03-27 10:48
+1 for the doc change.
msg290680 - (view) Author: Sanjay (Sanjay) * Date: 2017-03-28 03:38
ok I will update the doc
msg290830 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2017-03-30 07:44
New changeset 16f852345bcdec1bbb15e5363fad6b33bf960912 by Xiang Zhang (s-sanjay) in branch 'master':
bpo-29913: deprecate compare_networks() in documentation (GH-865)
https://github.com/python/cpython/commit/16f852345bcdec1bbb15e5363fad6b33bf960912
History
Date User Action Args
2022-04-11 14:58:44adminsetgithub: 74099
2017-03-30 07:45:06xiang.zhangsetstatus: open -> closed
resolution: fixed
stage: needs patch -> resolved
2017-03-30 07:44:31xiang.zhangsetmessages: + msg290830
2017-03-28 05:49:04python-devsetpull_requests: + pull_request765
2017-03-28 03:38:25Sanjaysetmessages: + msg290680
2017-03-27 10:48:20xiang.zhangsetmessages: + msg290579
2017-03-27 10:37:52ncoghlansetnosy: + docs@python
messages: + msg290577

assignee: docs@python
components: + Documentation, - Library (Lib)
stage: needs patch
2017-03-27 10:10:16xiang.zhangsetmessages: + msg290573
2017-03-27 09:44:12ncoghlansetmessages: + msg290572
2017-03-27 09:37:23Sanjaysetmessages: + msg290571
2017-03-27 08:55:06xiang.zhangsetnosy: + xiang.zhang
messages: + msg290569
2017-03-27 08:43:09serhiy.storchakasetnosy: + ncoghlan, pmoody
2017-03-27 07:25:27Sanjaysetpull_requests: + pull_request739
2017-03-27 07:23:35Sanjaycreate