(#1287) TextEnvelope is a real envelope #1458
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1458 +/- ##
============================================
- Coverage 89.49% 89.45% -0.05%
- Complexity 1644 1661 +17
============================================
Files 277 277
Lines 3970 3963 -7
Branches 210 209 -1
============================================
- Hits 3553 3545 -8
- Misses 385 386 +1
Partials 32 32
Continue to review full report at Codecov.
|
|
Job #1458 doesn't exist, can't set role |
|
@0crat in |
| public final int hashCode() { | ||
| return new Unchecked<>(this.origin).value().hashCode(); | ||
| public final boolean equals(final Object obj) { | ||
| return this.origin.equals(obj); |
There was a problem hiding this comment.
@victornoel Wouldn't this definition of equals cause problems? The operation seems to loose symmetric properties.
For example
new TextEnvelope(x).equals(x) == true, but not necessarily x.equals(new TextEnvelope(x))
There was a problem hiding this comment.
@andreoss this is a known limitation of equals actually when an interface has many different implementations. This subject has been discussed many times in the EO community, i.e., what does method equals mean for objects (you can search for "equals" in cactoos issues for example).
My take on it is that if you start having multiple implementations of the same interface, then you can't just expect equals to rely on some internal state, and thus, when you implement it, you have to rely on the exposed behaviour of the interface.
In the case of Text, it means that all implementations of Text should rely on Text.asString to implement equals, so in that case, the symmetry will be conserved. It is the same for MapOf, IterableOf and the other collection-related classes for example. Also sometimes it's better not to implement equals at all (e.g., Iterator).
There was a problem hiding this comment.
@andreoss see for example #1156 (comment) where I made the same kind of comment. Also I started a discussion on all that matter in #1241, don't hesitate to chime in.
|
@0crat status |
|
@victornoel This is what I know about this job in C63314D6Z, as in §32:
|
|
@andreoss see my changes and answers |
|
@rultor merge |
@victornoel OK, I'll try to merge now. You can check the progress of the merge here |
@victornoel Done! FYI, the full log is here (took me 9min) |
|
Job was finished in 3 hours, bonus for fast delivery is possible (see §36) |
This is for #1287.