yegor256 / cactoos-http

Object-Oriented HTTP Client
MIT License
37 stars 28 forks source link

(#64) HtAutoClosedResponse #79

Closed victornoel closed 5 years ago

victornoel commented 5 years ago

This is for #64.

@llorllale I made some design choices here, please review them carefully too :)

0crat commented 5 years ago

Job #79 is now in scope, role is REV

codecov-io commented 5 years ago

Codecov Report

Merging #79 into master will decrease coverage by 0.5%. The diff coverage is 90%.

Impacted file tree graph

@@            Coverage Diff             @@
##             master     #79     +/-   ##
==========================================
- Coverage        95%   94.5%   -0.5%     
- Complexity       69      75      +6     
==========================================
  Files            17      19      +2     
  Lines           180     200     +20     
  Branches         20      21      +1     
==========================================
+ Hits            171     189     +18     
- Misses            4       6      +2     
  Partials          5       5
Impacted Files Coverage Δ Complexity Δ
src/main/java/org/cactoos/http/HtResponse.java 72.72% <ø> (ø) 3 <0> (ø) :arrow_down:
...in/java/org/cactoos/http/HtAutoClosedResponse.java 100% <100%> (ø) 2 <2> (?)
...ava/org/cactoos/http/io/AutoClosedInputStream.java 87.5% <87.5%> (ø) 4 <4> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 52e8b09...9c2c0c7. Read the comment docs.

llorllale commented 5 years ago

@victornoel a couple of minor comments

victornoel commented 5 years ago

@llorllale it's done, thanks

0crat commented 5 years ago

This pull request #79 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

victornoel commented 5 years ago

@vzurauskas indeed, very good point, I removed all state from the class and only auto-close and forwards calls to read or close if reading or closing happens again.

vzurauskas commented 5 years ago

@victornoel Looks good. But Codecov's checks are failing, it seems to want more tests.

victornoel commented 5 years ago

@vzurauskas I've tried to increase coverage a bit, but I don't think I will be able to cover everything there because testing of InputStream is quite tricky. I've also noted your comment about this being a bit too much work, I agree, I will restrain myself in the future, thanks for the reminder.

@llorllale concerning coverage, I can add a @todo to introduce some abstraction to better test input streams implementation and their behaviour maybe?

llorllale commented 5 years ago

@victornoel @vzurauskas coverage checks are currently enforced only in cactoos-matchers :)

@vzurauskas waiting on your final verdict for this PR..

llorllale commented 5 years ago

@rultor merge

rultor commented 5 years ago

@rultor merge

@llorllale OK, I'll try to merge now. You can check the progress of the merge here

rultor commented 5 years ago

@rultor merge

@llorllale Done! FYI, the full log is here (took me 12min)

0crat commented 5 years ago

@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

sereshqua commented 5 years ago

@vzurauskas please make sure, that next time you will find at least 3 issues when doing a code as written in the policy

0crat commented 5 years ago

The job #79 is now out of scope

0crat commented 5 years ago

Payment to ARC for a closed pull request, as in §28: +10 point(s) just awarded to @llorllale/z

vzurauskas commented 5 years ago

@sereshqua Will do.

sereshqua commented 5 years ago

@0crat quality acceptable

0crat commented 5 years ago

Order was finished, quality is "acceptable": +15 point(s) just awarded to @vzurauskas/z

0crat commented 5 years ago

Quality review completed: +4 point(s) just awarded to @sereshqua/z