(#80) Read from streams in tests without closing #88
Conversation
|
Job #88 is now in scope, role is |
Codecov Report
@@ Coverage Diff @@
## master #88 +/- ##
=========================================
Coverage 95.02% 95.02%
Complexity 74 74
=========================================
Files 19 19
Lines 201 201
Branches 13 13
=========================================
Hits 191 191
Misses 7 7
Partials 3 3
Continue to review full report at Codecov.
|
|
This pull request #88 is assigned to @vzurauskas/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; there will be no monetary reward for this job |
|
@vzurauskas ping |
| * | ||
| * <p>There is no thread-safety guarantee. | ||
| * | ||
| * @since 0.1 |
There was a problem hiding this comment.
@victornoel Current version is 0.0.1.
There was a problem hiding this comment.
@vzurauskas yes, it was already released, so I can't use it with the since tag. Because I don't know what is going to be the next version, I used 0.1 since most of the cactoos artefacts were using that kind of versioning.
Let's ask @llorllale what he thinks.
I personally believe this tag is useless, see yegor256/qulice#1036 for a discussion on this :)
| public final class ReadBytes implements Bytes { | ||
|
|
||
| /** | ||
| * Scalar. |
There was a problem hiding this comment.
@victornoel It's clear from the name of the variable and also from its type that this is a scalar. Consider adding something useful to the description, for instance what this scalar represents.
| new TextOf( | ||
| new ReadBytes(new AutoClosedInputStream(closeable)) | ||
| ), | ||
| new TextIs("") |
There was a problem hiding this comment.
@victornoel Does it matter for this test whether Text of DeadInputStream is ""? The test is supposed to assert that the stream is closed after all bytes are read, which is the next assertion. Here I would just write something like new ReadBytes(new AutoClosedInputStream(closeable)).asBytes().
| * | ||
| * @since 0.1 | ||
| */ | ||
| public final class ReadBytes implements Bytes { |
There was a problem hiding this comment.
@victornoel Couldn't BytesOf from cactoos be used instead of this new class?
There was a problem hiding this comment.
@vzurauskas nope, that was the purpose of the todo solved here: BytesOf has the problem of closing the stream after reading it. I will update the javadoc and add a test to enforce this behaviour!
victornoel
force-pushed
the
80-read-stream-in-test-no-close
branch
from
8059e70 to
a65d7b4
Compare
|
@vzurauskas I've answered your comments and committed changes |
There was a problem hiding this comment.
@llorllale Looks good, but note the question about the @since version.
EDIT: Oh, except that the build is failing for some reason. @victornoel
|
@victornoel try rebasing on |
victornoel
force-pushed
the
80-read-stream-in-test-no-close
branch
from
a65d7b4 to
19720b8
Compare
|
@vzurauskas @llorllale it's fixed normally, but please read it again because I had forgotten some classes in my last push apparently |
|
@llorllale Looks good. |
|
@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 19min) |
|
@sereshqua/z please review this job completed by @vzurauskas/z, as in §30; the job will be fully closed and all payments will be made when the quality review is completed |
|
Code review was too long (16 days), architects (@llorllale) were penalized, see §55 |
|
The job #88 is now out of scope |
|
Payment to |
|
@0crat quality good |
|
Quality review completed: +4 point(s) just awarded to @sereshqua/z |
|
Order was finished, quality is "good": +20 point(s) just awarded to @vzurauskas/z |
This is for #80: