#764: Implement HeadInput #801
Conversation
|
Job #801 is now in scope, role is |
Codecov Report
@@ Coverage Diff @@
## master #801 +/- ##
===========================================
+ Coverage 83.36% 83.6% +0.23%
- Complexity 1297 1331 +34
===========================================
Files 232 242 +10
Lines 3541 3622 +81
Branches 202 211 +9
===========================================
+ Hits 2952 3028 +76
- Misses 542 546 +4
- Partials 47 48 +1
Continue to review full report at Codecov.
|
|
This pull request #801 is assigned to @krzyk/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 |
|
@proshin-roman I will close mine, don't worry :) |
|
@memoyil It wasn't necessary, as you could submit your changes even after applying mine ones: your pull request has a lot of ctors and test cases. |
|
|
||
| @Override | ||
| public InputStream stream() throws IOException { | ||
| return new HeadInputStream(this.origin.stream(), this.length); |
There was a problem hiding this comment.
@proshin-roman I'm wondering, why did you implement it using another InputStream? Is there any benefit from that? Wouldn't be using a ByteInputStream enough for this?
There was a problem hiding this comment.
@krzyk Even using ByteInputStream I would need to implement all this custom logic that I wrote in HeadInputStream. So, I would prefer to keep the current impl.
There was a problem hiding this comment.
@proshin-roman OK, your implementation is more robust but I think you lost more time on it. Using byte array would be faster :) - e.g. you wouldn't need skip and available
There was a problem hiding this comment.
@krzyk I just want to add that @proshin-roman's solution is in line with existing designs in org.cactoos.io (eg. TeeInput/TeeInputStream, LoggingOutput/LoggingOutputStream, etc.).
| adjusted = skip; | ||
| } | ||
| this.processed = this.processed + adjusted; | ||
| return this.origin.skip(adjusted); |
There was a problem hiding this comment.
@proshin-roman skip might return value smaller than adjusted, in that case processed might have incorrect number
There was a problem hiding this comment.
@krzyk thanks, I've adjusted the logic
| stream.skip(3L); | ||
| MatcherAssert.assertThat( | ||
| "Incorrect head of the input stream has been read", | ||
| new TextOf(stream).asString(), |
There was a problem hiding this comment.
@proshin-roman instead of asString() you should use TextHasString matcher
| stream.reset(); | ||
| MatcherAssert.assertThat( | ||
| "Reset didn't change the state", | ||
| new TextOf(stream).asString(), |
There was a problem hiding this comment.
@proshin-roman instead of asString() you should use TextHasString matcher
| new InputOf("readsHeadOfLongerInput"), | ||
| 5 | ||
| ) | ||
| ).asString(), |
There was a problem hiding this comment.
@proshin-roman instead of asString() you should use TextHasString matcher
| new InputOf(input), | ||
| 35 | ||
| ) | ||
| ).asString(), |
There was a problem hiding this comment.
@proshin-roman instead of asString() you should use TextHasString matcher
|
@llorllale looks good to merge |
|
@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 10min) |
|
@elenavolokhova/z please review this job completed by @krzyk/z, as in §30; the job will be fully closed and all payments will be made when the quality review is completed |
|
The job #801 is now out of scope |
|
Payment to |
|
@0crat quality good |
|
Quality review completed: +8 point(s) just awarded to @elenavolokhova/z |
Pull request for issue #764.