Skip to content
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

Return false when comparing different python objects holding the same version tag #2838

Merged

Conversation

bgartzi
Copy link
Contributor

@bgartzi bgartzi commented Jan 11, 2024

Hello!

While I was working on another issue, I found that the inequality operator of python rpm.ver objects would not work as I was expecting to. != would always return True if the compared objects were not the actual same python object. That was leading to situations as

    (v := rpm.ver('1.0')) != v        # False, we are comparing the object with itself
    rpm.ver('1.0') != rpm.ver('1.0')  # True, although they hold the same version tag string

I'm not sure whether that was the intended behavior or not. It was not quite what I expected when I tried applying the operator. I didn't research the topic really exhaustively, but I couldn't find related issues at least in the PRs of this repo.

Anyway, this patch adds the PY_NE case to the tp_richcompare slot function and adds some extra checks to a related existing test.

Thanks for the attention! 😄

cc: @luckyh

The inequality operator of the python ver objects would always return
True if the compared objects were not the actual same python object.
That was leading to situations as

    (v := rpm.ver('1.0')) != v        # False
    rpm.ver('1.0') != rpm.ver('1.0')  # True

This commit aims to support the inequality operator among different
python objects that hold the same version the (opposite) way we expect
the equality operator to work. To do so, it adds the PY_NE case to the
tp_richcompare slot function and adds extra checks to a related
rpmpython test case.

Signed-off-by: Beñat Gartzia <[email protected]>
@pmatilai
Copy link
Member

pmatilai commented Jan 11, 2024

Oh, nice catch. I guess I thought/assumed implementing EQ will do the right thing with NE too because .. well, it's just the opposite, right? Guess not.

Thanks for the patch!

@pmatilai pmatilai added python Python bindings bug labels Jan 11, 2024
@pmatilai pmatilai merged commit f1b68c9 into rpm-software-management:master Jan 11, 2024
1 check passed
@bgartzi
Copy link
Contributor Author

bgartzi commented Jan 11, 2024

Yeah, I'd have assumed the same thing actually, that's why it was weird to me to see != wasn't acting as a complement of == 😅(now I've just found why that was intended [0]). I guess that it just implements a is not by default? I'm not sure.

No problem, thank you for the quick reply!

[0] https://peps.python.org/pep-0207/#proposed-resolutions (point 3)

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

Successfully merging this pull request may close these issues.

2 participants