Test if getHash is stable - #243
Conversation
There was a problem hiding this comment.
What is the benefit of adding this? It works just as well without no? What I'd expect to see somewhere if you add such code is polymorphic usage, though that is not happening here, and I don't see how it would happen either.
There was a problem hiding this comment.
Benefit? So you can't forget it in other subclasses? See the other patch. Are we really starting the same discussion again?
There was a problem hiding this comment.
That's a rather small benefit, which is voided if you consider that these tests abuse inheritance. If you don't agree inheritance is abused, fine. This commit does not make it worse, so I don't object to this. I do however suggest you try to understand why the usage inheritance here is bad.
There was a problem hiding this comment.
I try to understand that but I see no argument. Instead I see either pointless code duplication or dependency injection hell, both leading to a harder to maintain, more fragile code base.
|
Generally looks good. Wondering how I ever thought this inheritance thing was a good idea. |
|
What the heck is wrong with inheritance? |
There was a problem hiding this comment.
Why does this refer to EntityIdTest?
There was a problem hiding this comment.
It's a comment targeted at devs possibly touching this code. Both "is a safeguard to make sure [...] is not changed unintentionally".
There was a problem hiding this comment.
That seems to imply we should have such references with all serialization stability tests to all other serialization stability tests - guess this is not what you want though? I'm not sure what the added value is, and it seems pretty arbitrary to do it just here.
|
See #244 for another refactoring which removes inheritance in this context completely. |
|
I have seen #244. I don't understand why we are wasting so much time for this (also see #132, wmde/WikibaseQueryEngine#112 and wmde/WikibaseQueryEngine#114 which are all nothing but a total waste of my time). I will not merge #244 but I will not object it. If it does the exact same tests, who cares how it is implemented in the end. But still. Why the heck is code duplication better than plain simple inheritance? I don't get that. |
Test if getHash is stable
I do, and so should you.
If you state that as a general rule, then I will certainly disagree with it. However in this case I'm not seeing the duplication, I just see an abstract method that's implemented in one place and never polimorphically called. There is a lot of subtlety to each situation that one needs to hold into account to create a good design. |
|
Speaking of generic rules:
If you have a problem with generic rules allow me to have a problem with generic rules too.
I'm talking about #244, #132, wmde/WikibaseQueryEngine#112, wmde/WikibaseQueryEngine#114 and some older where we discussed the same. I'm deeply alienated by that "war against inheritance" I see in more and more pull requests.
I already explained that, see #241. |
This is split from #132.