#881 equals and hashCode for CollectionEnvelope #894
Conversation
|
Job #894 is now in scope, role is |
Codecov Report
@@ Coverage Diff @@
## master #894 +/- ##
============================================
+ Coverage 86.06% 87.15% +1.09%
- Complexity 1435 1467 +32
============================================
Files 258 259 +1
Lines 3767 3791 +24
Branches 213 213
============================================
+ Hits 3242 3304 +62
+ Misses 475 439 -36
+ Partials 50 48 -2
Continue to review full report at Codecov.
|
| other.getClass(), Collection.class | ||
| ).value() > -1, | ||
| () -> new InheritanceLevel( | ||
| other.getClass(), CollectionEnvelope.class |
There was a problem hiding this comment.
@Vatavuk I believe this isn't necessary because CollectionEnvelope is a Collection already
There was a problem hiding this comment.
@llorllale InheritanceLevel measures inheritance distance between base and derived class. It cannot determine wether a class is instance of Collection.
I'we removed first condition comparing (other.getClass(), Collection.class) because it always evaluates to false.
There was a problem hiding this comment.
@llorllale the end result is that we will only compare classes that are related to CollectionEnvelope
There was a problem hiding this comment.
@Vatavuk We should be comparing Collections here, not CollectionEnvelopes.
I think I understand the problem now: InheritanceLevel does not check if a class implements an interface (maybe it shouldn't).
We're missing tests that compare CollectionEnvelope against regular JDK Collections. I believe they will fail with the current implementation. So we either need to a) add an instanceof (against our principles) or b) build a new type that checks if a class implements/extends an interface, or c) modify InheritanceLevel to take into account (b).
Please leave a puzzle for this and we'll merge.
| @@ -147,4 +147,48 @@ public String toString() { | |||
| return this.col.value().toString(); | |||
| } | |||
|
|
|||
| @Override | |||
| public final boolean equals(final Object other) { | |||
There was a problem hiding this comment.
@Vatavuk this equals is missing the null-check
There was a problem hiding this comment.
@llorllale fixed in d4538f8
|
@Vatavuk left you a couple of comments, see above. Great PR. |
|
@llorllale fixed and added a comment regarding |
|
@llorllale added a puzzle |
|
@Vatavuk there's a problem with the puzzle's format |
|
@llorllale fixed |
|
@rultor merge |
@llorllale OK, I'll try to merge now. You can check the progress of the merge here |
@llorllale Done! FYI, the full log is here (took me 15min) |
|
The job #894 is now out of scope |
Introduced
equalsandhashCodeinCollectionEnvelopefor #881