Skip to content

bpo-20825: Containment test for ip_network in ip_network. Patch by Michel Albert.#4065

Merged
pitrou merged 5 commits into
python:masterfrom
csabella:bpo20825
Oct 22, 2017
Merged

bpo-20825: Containment test for ip_network in ip_network. Patch by Michel Albert.#4065
pitrou merged 5 commits into
python:masterfrom
csabella:bpo20825

Conversation

@csabella

@csabella csabella commented Oct 20, 2017

Copy link
Copy Markdown
Contributor

Patch by Michel Albert.

https://bugs.python.org/issue20825

Comment thread Lib/ipaddress.py Outdated
return (self.network_address.is_multicast and
self.broadcast_address.is_multicast)

def _containment_check(self, check, other):

@pitrou pitrou Oct 21, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of switching on check, how about having something like:

@classmethod
def _is_subnet_of(cls, a, b):
    # Implementation here

def subnet_of(self, other):
    return self._is_subnet_of(self, other)

def supernet_of(self, other):
    return self._is_subnet_of(other, self)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I made these changes, except I made is_subnet_of a staticmethod since it didn't use cls.

@pitrou

pitrou commented Oct 21, 2017

Copy link
Copy Markdown
Member

Thanks @csabella . Please note you'll have to add a NEWS file using the blurb utility.

Comment thread Lib/ipaddress.py Outdated
raise TypeError("{} and {} are not of the same version"
.format(a, b))
# Dealing with another network.
if hasattr(b, 'network_address') and hasattr(b, 'broadcast_address'):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since _is_subnet_of can be called both ways, you should also test the type for a.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@pitrou

pitrou commented Oct 22, 2017

Copy link
Copy Markdown
Member

Note the CI failures are unrelated. An external NNTP server we use in our test suite seems to have issues...

@pitrou

pitrou commented Oct 22, 2017

Copy link
Copy Markdown
Member

I took the liberty of making small last-minute changes. Now I'm waiting for CI results again :-)

@pitrou
pitrou merged commit 91dc64b into python:master Oct 22, 2017
@csabella

Copy link
Copy Markdown
Contributor Author

Thank you!

@csabella
csabella deleted the bpo20825 branch October 22, 2017 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants