Skip to content

bpo-38335 simplify the overlap function for IpNetwork #16519

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

s-sanjay
Copy link
Contributor

@s-sanjay s-sanjay commented Oct 1, 2019

@s-sanjay
Copy link
Contributor Author

s-sanjay commented Oct 1, 2019

no news entry needed as there is no behavior change

Copy link
Contributor

@remilapeyre remilapeyre left a comment

Choose a reason for hiding this comment

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

Hi @s-sanjay, you should not move the method around in the file while changing them as it make reviewing the changes harder. Can you please move them back where they were?

@@ -1511,7 +1512,6 @@ def __init__(self, address, strict=True):
self.hosts = self.__iter__

@property
@functools.lru_cache()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you removed this decorator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because self.network_address.is_global which in turn calls is_private already has a cache. There are pros and cons for example if someone removes cache there then suddenly this would loose caching behavior and also this having its own cache frees up the other cache. Should I add it back ?

@s-sanjay
Copy link
Contributor Author

@remilapeyre yes, agreed I avoid moving methods around but in this case the is_private method was deleted from child classes and moved to the base class and the overlap method was moved closer to the subnet_of and supernet_of functions because overlap and them are related terms and the overlap method simply delegates the call to these two methods. I thought this small refactor will help greatly with future readability of the code. what do you think ? should I put it in the old place ?

@python-cla-bot
Copy link

The following commit authors need to sign the Contributor License Agreement:

CLA signed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants