Skip to content

Conversation

@yihong0618
Copy link
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>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
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
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
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
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
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
Contributor Author

yihong0618 commented Nov 22, 2025

closing as discuss~ cc @picnixz

@yihong0618 yihong0618 closed this Nov 22, 2025
@picnixz
Copy link
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
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