Skip to content

gh-141647: attribute breaks __eq__ behaviour of IPv4Network and IPv6Network#141651

Closed
yihong0618 wants to merge 5 commits into
python:mainfrom
yihong0618:hy/close_issue_141647
Closed

gh-141647: attribute breaks __eq__ behaviour of IPv4Network and IPv6Network#141651
yihong0618 wants to merge 5 commits into
python:mainfrom
yihong0618:hy/close_issue_141647

Conversation

@yihong0618
Copy link
Copy Markdown
Contributor

@yihong0618 yihong0618 commented Nov 17, 2025

compore to other __eq__ always use subclass check

Signed-off-by: yihong0618 <zouzou0208@gmail.com>
Signed-off-by: yihong0618 <zouzou0208@gmail.com>
Comment thread Lib/ipaddress.py Outdated
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Comment thread Misc/NEWS.d/next/Library/2025-11-17-17-06-56.gh-issue-131647.tyafol.rst Outdated
Comment thread Lib/test/test_ipaddress.py Outdated
Comment thread Lib/test/test_ipaddress.py Outdated
Comment thread Lib/test/test_ipaddress.py Outdated
Comment thread Lib/test/test_ipaddress.py Outdated
Comment thread Lib/test/test_ipaddress.py Outdated
Comment thread Lib/test/test_ipaddress.py Outdated
Comment thread Lib/test/test_ipaddress.py Outdated
Comment thread Lib/test/test_ipaddress.py Outdated
yihong0618 and others added 2 commits November 22, 2025 18:29
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Signed-off-by: yihong0618 <zouzou0208@gmail.com>
Copy link
Copy Markdown
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Actually, we also have a similar issue in _BaseNetwork.__contains__. version is being checked before the other type. Likewise, _BaseNetwork.address_exclude checks version before checking the type of other. And there are many more methods which check other.version before checking other. I will take care of this separately.

@yihong0618
Copy link
Copy Markdown
Contributor Author

Actually, we also have a similar issue in _BaseNetwork.__contains__. version is being checked before the other type. Likewise, _BaseNetwork.address_exclude checks version before checking the type of other. And there are many more methods which check other.version before checking other. I will take care of this separately.

Thank you very much again.

@picnixz
Copy link
Copy Markdown
Member

picnixz commented Nov 22, 2025

Urhg, there are other places where __eq__ and __lt__ are wrong. And other functions as well. If it's alright for you, I will just extract the tests you wrote and extend them (that means closing this PR but you'll still be credited). Is it fine? (I can also split the work into two but the NEWS entry will be the same so I prefer having one PR). If you prefer having a commit under your name for contribution purposes, I will first open my PR, merge yours, and then merge mine at the same time.

@yihong0618
Copy link
Copy Markdown
Contributor Author

Urhg, there are other places where __eq__ and __lt__ are wrong. And other functions as well. If it's alright for you, I will just extract the tests you wrote and extend them (that means closing this PR but you'll still be credited). Is it fine? (I can also split the work into two but the NEWS entry will be the same so I prefer having one PR). If you prefer having a commit under your name for contribution purposes, I will first open my PR, merge yours, and then merge mine at the same time.

of course its fine, I like that.

@yihong0618
Copy link
Copy Markdown
Contributor Author

yihong0618 commented Nov 22, 2025

closing as discuss~ cc @picnixz

@yihong0618 yihong0618 closed this Nov 22, 2025
@picnixz
Copy link
Copy Markdown
Member

picnixz commented Nov 22, 2025

Actually, it's even more complex... we have mixed type equalities that are allowed, so we cannot just check with _BaseAddress.........

@yihong0618
Copy link
Copy Markdown
Contributor Author

Actually, it's even more complex... we have mixed type equalities that are allowed, so we cannot just check with _BaseAddress.........

yes, also think about that, but old code check it, so in my patch I think its fine.

and this is not doc, I wonder user own their risk?

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.

2 participants