Added eq, ne, gt etc. methods.

[Tom1380]

This is done for convenience and to be friendlier to beginners.

Closes #2155.

[mejrs]

Thanks!

Can you add tests for these? 😃

Also, I'm not sure about the names for these. I'm tempted to go with a.eq(b) over a.is_eq(b) and so on. @davidhewitt
since you were the one that suggested these - what do you think?

[birkenfeld]

Also needs an entry in the changelog, please.

[Tom1380]

@mejrs sure, I'd be happy to add tests.

I tried to find where rich_compare itself is tested, but I couldn't find it.
Can you point me to where I can find it? So I can add tests right below it.

[Tom1380]

@mejrs Should I add the tests as documentation tests?

[mejrs]

@mejrs Should I add the tests as documentation tests?

I would recommend adding a new test at the bottom of the file where you create a vec with a bunch of different python objects and loop through every possible pair against every compare op, and check whether they give the expected results. That is probably a bit much for a doc example 😉

[davidhewitt]

Thanks! Regular tests would be better than doctests please; we don't measure coverage for doctests yet.

Also, I'm not sure about the names for these. I'm tempted to go with a.eq(b) over a.is_eq(b) and so on. @davidhewitt
since you were the one that suggested these - what do you think?

The only thing that makes me uneasy about that is that then a.eq(b) clashes with the PartialEq implementation, which uses pointer equality rather than Python equality. I think having the notably different names is a good thing? Or maybe we should consider changing the PartialEq implementation. (Though what to do if __eq__ raises an exception? Panic?)

[mejrs]

Oh, that is unfortunate. But having is_eq and eq methods available is also uneasy for me.

In hindsight it may have been better to not have the PartialEq impls and have a ptr_eq method instead.

[birkenfeld]

Yeah I agree that obj1 == obj2 just comparing pointers is inviting confusion. I'd rather have that exposed as a.is(b) as in Python (ptr_eq is also ok but hard to find when coming from the Python side). Then the way would be clear for eq etc. methods using the richcmp functions, and PartialEq is removed.

The question is, can this have further repercussions than an easy-to-fix compile error? If we're not currently also providing Eq, at least it wasn't possible to put objects into HashMaps.

[davidhewitt]

I would be happy to remove the current PartialEq implementation. I'm pretty convinced that pointer equality for that is surprising.

[mejrs]

I've thought about it some more and I really don't like the PartialEq either.

So, I think the plan is?

• Remove PartialEq
• Add PyAny::is(other)
• Rename all the methods this PR added from is_<op> to just <op>

[davidhewitt]

We may want to mention the removal in the migration guide, just in case it causes any surprises for users. (Given it's not possible to deprecate a trait implementation as far as I know.)

[birkenfeld]

I did the PartialEq related stuff in #2183, so this PR just needs some tests and changelog entry to be ready.

[Tom1380]

Please let me know if you think the tests are done right, I'm not sure that Rust's cmp logic is the same as Python's.

[birkenfeld]

I wonder if we shouldn't go one step further here and do the is_true call in the functions.

I know that richcmp methods can return anything, but that is already quite esoteric, and usability here will be much better if only one ? or unwrap is needed.

Those who need the general result can still use rich_compare after all.

[davidhewitt]

I wonder if we shouldn't go one step further here and do the is_true call in the functions.

I would support that.

[Tom1380]

I wonder if we shouldn't go one step further here and do the is_true call in the functions.

Done.

[Tom1380]

I think it's ready. Please review it.

[birkenfeld]

I think this is good to go. Don't know how the coverage could go down, all methods are tested.

[davidhewitt]

Codecov seems to do this weird thing where any PRs with the originating fork branch as main it takes that coverage as our current CI coverage. I noticed this the other day but haven't really had time to investigate or figure out why...

[birkenfeld]

I also saw that, since rustfmt split the assert as much as possible, it complained about the arguments for the assert error format not being covered. While true, that is unavoidable for working tests :)

[davidhewitt]

True, yes! Perhaps the upstream macro expansion should consider having the #[no_coverage] attribute added on the assertion failure branch?!