Thank you! On Saturday, 2 September 2023 at 13:44:46 UTC+2 Oscar Benjamin wrote:
> On Sat, 2 Sept 2023 at 12:15, 'Martin R' via sage-devel > <sage-...@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/5bcae747-1901-4616-936f-a509b0cddbdan%40googlegroups.com.
