(#1230) ListIteratorOf can be mutable and immutable, depending on origin #1231
Conversation
|
Job #1231 is now in scope, role is |
Codecov Report
@@ Coverage Diff @@
## master #1231 +/- ##
============================================
+ Coverage 89.19% 89.37% +0.18%
- Complexity 1668 1671 +3
============================================
Files 280 280
Lines 4006 4009 +3
Branches 213 213
============================================
+ Hits 3573 3583 +10
+ Misses 399 392 -7
Partials 34 34
Continue to review full report at Codecov.
|
|
This pull request #1231 is assigned to @victornoel/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @paulodamaso/z (the architect) right in this ticket; if you decide that this PR should not be accepted ever, also inform the architect; this blog post will help you understand what is expected from a code reviewer; there will be no monetary reward for this job |
There was a problem hiding this comment.
@fanifieiev @paulodamaso I question the very existence of ListOteratorOf, it does nothing except wrapping a ListIterator: either we should remove it or we can rename it ListIteratorEnvelope and make it abstract. The later could make sense I believe. Please decide @paulodamaso.
| new Assertion<>( | ||
| "List iterator returns incorrect previous index", | ||
| "List Iterator must return previous index", |
There was a problem hiding this comment.
@fanifieiev why is ListIteratorOf tested in ListEnvelopeTest? I think those tests should be in their own file (and removed from here).
There was a problem hiding this comment.
@victornoel I agree with your concern, I just left them where they were previously.
There was a problem hiding this comment.
@fanifieiev so please move them in their own file to properly identify what is being tested
| @@ -167,7 +166,6 @@ public void removeThrowsErrorForImmutableListIterator() { | |||
| return 0; | |||
| }, | |||
| new Throws<>( | |||
| "Iterator is read-only and doesn't allow removing items", | |||
There was a problem hiding this comment.
@fanifieiev why remove those parts of the tests?
There was a problem hiding this comment.
@victornoel If you look at the test and the following stacktrace, you can see that the exception is thrown by the collection returned by Collections.unmodifiableList, that is why it throws exception with no message.
There was a problem hiding this comment.
@fanifieiev I better understand, thank you, but why is ListIteratorNoNulls based on an immutable version of ListIterator? I think that it is coherent we the changes we are making that ListIteratorNoNulls is to be only concerned with nulls, not with mutability. Please fix it or add a todo to continue the work.
There was a problem hiding this comment.
@victornoel I did not catch your concern. The ListIteratorOf I changed is not based on the immutable version of ListIterator.
Could you provide details regarding you concern?
There was a problem hiding this comment.
I'm talking about ListIteratorNoNulls: those tests on which we are currently commenting seem to validate that it is immutable but I don't think it is in the spirit of the changes that are being done to the ListIterator type hierarchy in this PR: either change ListIteratorNoNulls to be coherent with the changes you are currently making, or add a todo in order for someone to do it.
There was a problem hiding this comment.
@fanifieiev I think we spent enough time on this PR, please add todos and let ARC validate them by merging.
There was a problem hiding this comment.
@fanifieiev one todo is enough, no need to multiplicate them btw :)
There was a problem hiding this comment.
@victornoel There is one thing that concerns me.
Let's say we will make those mentioned classes mutable. Let's take Sorted as an example:
List<Integer> sorted = new Sorted<>(2, 1, 0, 5);
Sorted list is now sorted.
Let's add one more element:
sorted.add(1);
Sorted list is not sorted anymore.
You got the idea of my concern?
There was a problem hiding this comment.
@fanifieiev that's why you should leave a todo and let the next job performer think about this problem. Maybe we will conclude that Sorted can't be mutable, but that's out of scope of the present issue. Personally I believe the real problem is that it's not relevant to decorate collection to sort them, but that's only my opinion ;) Also, nothing prevent to implement Sorted using a Java ordered list and implement insertion in a sorted manner.
There was a problem hiding this comment.
@victornoel Ok, I have just pushed one more commit with 'todo'. Please have a look.
@victornoel I would keep it as an abstract class ONLY if we really need it, otherwise would remove. |
|
@paulodamaso it's good on my side, please take a look and as the above discussion as well and to my previous comment too. |
|
@victornoel @fanifieiev It's good, thank you. |
|
@rultor merge |
@paulodamaso OK, I'll try to merge now. You can check the progress of the merge here |
@paulodamaso Done! FYI, the full log is here (took me 12min) |
|
@sereshqua/z please review this job completed by @victornoel/z, as in §30; the job will be fully closed and all payments will be made when the quality review is completed |
|
The job #1231 is now out of scope |
|
Payment to |
|
@victornoel please make sure you would try to find at least 3 issues during next CR, thanks |
|
@sereshqua well actually I did if you take the global review comment into account :) |
|
@victornoel good point, Policy does not give clear instruction and its treatment can be subjective; from one point there are 3 comments: 1 global and 2 exact (pointing the code out), |
|
@sereshqua hehe, do as you see fit, it's not very important in the end, typically there is no ambiguities :) |
|
@victornoel sure, previously i was counting only comments that were pointing out the code, so I would keep my qa verdicts consistent |
|
@0crat quality acceptable |
|
Order was finished, quality is "acceptable": +15 point(s) just awarded to @victornoel/z |
|
Quality review completed: +4 point(s) just awarded to @sereshqua/z |
This PR is for 1230
The ListIteratorOf should support mutability, if the origin supports