IteratorOfLongs and IteratorOfShorts #854
Conversation
|
Job #854 is now in scope, role is |
|
This pull request #854 is assigned to @Vatavuk/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @llorllale/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 |
|
@victornoel build is not passing, there are some formatting errors |
Codecov Report
@@ Coverage Diff @@
## master #854 +/- ##
============================================
+ Coverage 85.53% 85.55% +0.02%
- Complexity 1392 1396 +4
============================================
Files 251 253 +2
Lines 3698 3704 +6
Branches 211 209 -2
============================================
+ Hits 3163 3169 +6
Misses 487 487
Partials 48 48
Continue to review full report at Codecov.
|
|
@Vatavuk sorry about that, it seems this week is not my week ;) |
| this.iteratorWithFetchedElements().next(); | ||
| } | ||
|
|
||
| private IteratorOfLongs iteratorWithFetchedElements() { |
There was a problem hiding this comment.
@victornoel this should be static method, but better approach would be to remove it completely and move its implementation back to tests using different iterator size in each test. This would remove dependency between tests and provide more test diversity.
There was a problem hiding this comment.
I agree, I also did the same for the other IteratorOfXXX tests (actually some of them were already as you propose).
| this.iteratorWithFetchedElements().next(); | ||
| } | ||
|
|
||
| private IteratorOfShorts iteratorWithFetchedElements() { |
There was a problem hiding this comment.
@victornoel same comment as for IteratorOfLongsTest
victornoel
force-pushed
the
838
branch
2 times, most recently
from
9cf061c to
6caaae6
Compare
|
@Vatavuk it is fixed now and rebased on master |
|
@llorllale good to merge |
|
@llorllale I think this should be merged |
| * | ||
| * <p>There is no thread-safety guarantee.</p> | ||
| * | ||
| * @since 0.32 |
There was a problem hiding this comment.
@victornoel should be 0.34 now
| * | ||
| * <p>There is no thread-safety guarantee.</p> | ||
| * | ||
| * @since 0.32 |
There was a problem hiding this comment.
@victornoel should be 0.34 now
| /** | ||
| * Tests for {@link IteratorOfLongs}. | ||
| * | ||
| * @since 0.32 |
| /** | ||
| * Tests for {@link IteratorOfShorts}. | ||
| * | ||
| * @since 0.32 |
|
@victornoel there are a lot of irrelevant changes in this PR. Although they're small, they're introducing too much noise. Let's keep PRs tightly focused in the future please |
|
@victornoel also see my comments regarding |
|
@llorllale duly noted for the extra changes, next time I will open issues. The |
|
@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 9min) |
|
@elenavolokhova/z please review this job completed by @Vatavuk/z, as in §30; the job will be fully closed and all payments will be made when the quality review is completed |
|
The job #854 is now out of scope |
|
Payment to |
|
@0crat quality good |
|
Quality review completed: +8 point(s) just awarded to @elenavolokhova/z |
This is for #838.
This:
IteratorOfLongsandIteratorOfShortsIterableOfLongsandIterableOfShortsIteratorOfXXX(because it was so easy to do so and blatantly incorrect)IteratorOfXXX(because I spent some time trying to understand something simple: that their role was to convert from primitive to objects)IteratorOfXXXinIterableOfXXXI realise this is a bit more than the job asked, but all the other changes I made, I had to make them for the code I was introducing anyway.