gh-111442: Correct names for all 0's and all 1's hosts in Address objects, and deprecate incorrect names. #132421
Conversation
… looking for broadcast_address
last_address to both IPv4Network and IPv6Network.
…twork_address and subnet_router_anycast_address
|
This is my first pull request to Python, or to be honest, any "major" OSS project in quite some time. Please give me any advice / pointers / recommendations / insults as I'd also like to close out #125641 at some point. |
There was a problem hiding this comment.
Thanks for putting togethor this PR! I noticed that there hasn't yet been any discussion on the issue or on DPO about this feature. It would be a good idea to open a thread on https://discuss.python.org/c/ideas/6 to get some feedback. In particular, it would be good to get a consensus about whether it makes sense to deprecate the existing attributes on IPv6Network.
I've added some general review comments below.
| .. attribute:: network_address | ||
| .. attribute:: broadcast_address |
There was a problem hiding this comment.
These shouldn't be removed quite yet - they'd need be documented during a deprecation period.
| # is a given. | ||
| if last.broadcast_address >= net.broadcast_address: | ||
| if last.last_address >= net.last_address: |
There was a problem hiding this comment.
Changing these names internally feels like an unnecessary cosmetic change. These make the diff harder for a core dev to review and may create backport conflicts in the future. Plus there's a risk of accidentally introducing a bug. It might be a good idea to revert these for now.
There was a problem hiding this comment.
I see it both ways. This is touching a bunch of lines for seemingly no effect, but it also means someone new to reading the codebase should hopefully not be confused by things like why a IPv6Network has a test for its broadcast_address.
If everyone hates this, its obviously not a problem to revert the internal naming.
| @functools.cached_property | ||
| def broadcast_address(self): | ||
| return self.last_address | ||
|
|
||
| @functools.cached_property | ||
| def network_address(self): | ||
| return self.first_address |
There was a problem hiding this comment.
Hmm, is there value in making these cached_propertys when they're just surrogates for last_address/first_address , which already are cached_propertys?
There was a problem hiding this comment.
This made sense when you wrote it.
But they are slightly modified now to have different behaviour for /31's and /127's, reflecting the lack of a broadcast_address on those sized networks.
| @@ -1393,16 +1393,6 @@ def testGetBroadcast(self): | |||
| self.assertEqual(int(self.ipv4_network.broadcast_address), 16909311) | |||
| self.assertEqual(str(self.ipv4_network.broadcast_address), '1.2.3.255') | |||
|
|
|||
| self.assertEqual(int(self.ipv6_network.broadcast_address), | |||
| 42540616829182469451850391367731642367) | |||
There was a problem hiding this comment.
These tests should stay for now, even if it's decided that broadcast_address should be deprecated.
There was a problem hiding this comment.
Readded with extra deprecation checks.
| self.assertIn('hostmask', self.ipv6_network.__dict__) | ||
| self.assertIn('broadcast_address', self.ipv6_interface.network.__dict__) | ||
| self.assertIn('hostmask', self.ipv6_interface.network.__dict__) | ||
|
|
There was a problem hiding this comment.
Tests should be added for all the new attributes. Additionally, you'll need to update the tests that now raise deprecation warnings.
There was a problem hiding this comment.
Added tests that check for the deprecation warnings when used on IPv6Networks.
I've created https://discuss.python.org/t/ipaddress-deprecate-network-address-broadcast-address-from-ipv6networks-add-subnet-router-anycast-and-terminology-cleanups/88099 to discuss this. |
Co-authored-by: Brian Schubert <brianm.schubert@gmail.com>
|
All commit authors signed the Contributor License Agreement. |
Co-authored-by: Brian Schubert <brianm.schubert@gmail.com>
Co-authored-by: Brian Schubert <brianm.schubert@gmail.com>
Co-authored-by: Brian Schubert <brianm.schubert@gmail.com>
Docs cleanup Co-authored-by: Brian Schubert <brianm.schubert@gmail.com>
…nd test for deprecation warnings (for now)
Use the correct langauge for the all 0's and all 1's addresses in networking.
For example, IPv6Networks don't have a network or a broadcast address. While the subnet_router_anycast address has all the host bits set to 0, you would expect to see packets on the wire addressed to it, where in IPv4 you would not expect to see packets addressed to the network_address. The all 1's host address in IPv6 has no special meaning.
This change deprecates the use of network_address or broadcast_address for IPv6Networks, and introduces the subnet_router_anycast_address property.
Internally, it does this by changing most previous occurances in the NetworkBase class for network_address to first_address and broadcast_address to last_address, and using the address family specific names in each address families class.
📚 Documentation preview 📚: https://cpython-previews--132421.org.readthedocs.build/