Move HashArray::equals to SnakList::equals #713
Conversation
thiemowmde
force-pushed
the
equalHashArrays
branch
from
ea52b89
to
32bc1be
Compare
|
please rebase. +2 would merge |
thiemowmde
force-pushed
the
equalHashArrays
branch
from
a5ae477
to
fa615f6
Compare
|
Manually rebased, and made sure the changes are identical to before. |
| 'different objects with different content are not equal' => [ | ||
| $oneSnak, | ||
| new SnakList( [ new PropertyNoValueSnak( 2 ) ] ), | ||
| false |
There was a problem hiding this comment.
I'd rather see this as a separate test (as in separate method) instead of squashing both test case data, and the expected result in the data provider method but whatever.
There was a problem hiding this comment.
Both are valid approaches, and even if I agree that splitting test cases is the better approach in many cases, I disagree in this case. Readability is key here, and the names of these test cases almost read like a poem. This poem will be destroyed if the last one is not an array key any more but a method name. I don't think this is worth it in this case, if the only benefit is that five booleans can be removed.
There was a problem hiding this comment.
On the off-topic side: if you really want to compare source code to literature works you might consider using "prose" in comparison. I wouldn't consider "readability" or "clarity" an obvious value of "good" poem :)
There was a problem hiding this comment.
I can totally read this out aloud as a poem. Hm. Let me add this as a proposal to the Lightning Talk board.
This goes along with #714 (and will run into a merge conflict – feel free to merge whichever first, I will do the rebase). Both are split from #702 to make that big patch it easier to review later.
Note that I rewrote the tests entirely. This is also the main reason for this patch, to make sure SnakList is properly covered with tests before we are applying the big change in #702.