On Sat, 2 Sept 2023 at 12:15, 'Martin R' via sage-devel
<sage-devel@googlegroups.com> wrote:
>
> On Saturday, 2 September 2023 at 11:39:12 UTC+2 Oscar Benjamin wrote:
>
> On Sat, 2 Sept 2023 at 08:44, 'Martin R' via sage-devel
> <sage-...@googlegroups.com> wrote:
>
> ...
>
> It is easy for this sort of thing to be overlooked in test code and in fact
> messing with __eq__/__ne__ (more so __eq__) can invalidate much of the
> test suite so I would tread carefully.
>
> Could you provide an example? I would think that in such a case a doctest
> should catch the problem - what other purpose would a doctest have?
It is not so much an issue with doctests but in SymPy most tests are
pytest-style tests that look something like:
def test_A():
assert A(1) == A(10)
assert A(2) == A(7)
assert A(3) != A(5)
...
Now if you change your class A to look like:
class A:
def __eq__(self, other):
return True
def __ne__(self, other):
return True
Then your test suite will pass regardless of what any of the rest of
the code does because every == or != returns True all the time.
Obviously you would not literally write "return True" but an
undetected bug having that sort of effect could invalidate many of the
tests so you have to be very careful how you write and test methods
like __eq__ and __ne__. That would make me nervous about wholesale
removal of __ne__ without some careful checking because it is not
*always* the case that removing __ne__ will have no effect.
The first contributor who submitted a PR for this in SymPy did not do
careful checking and did not seem to understand all of the
implications of removing __ne__ and so I did not trust the PR to be
merged regardless of the tests eventually passing (when the __ne__
methods that were still needed were restored). Subsequently the PR was
rewritten by a more experienced contributor and then merged. Some
other contributors seemed not to appreciate my concern over this at
the time but at least for SymPy removing some harmlessly redundant
__ne__ methods brought close to zero benefit in exchange for a
moderate risk of introducing bugs.
--
Oscar
--
You received this message because you are subscribed to the Google Groups
"sage-devel" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to sage-devel+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/sage-devel/CAHVvXxQrhhbDJSijpb5Mo4071FPsg6vU13TT_SbkPK%2B2O6GJLg%40mail.gmail.com.
