ilyakharlamov commented 5 years ago

( #42 ) HtHead can be broken

Adds unit test for buffer tearing
Replaces the double loop with a scanner

0crat commented 5 years ago

Job #69 is now in scope, role is REV

0crat commented 5 years ago

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

codecov-io commented 5 years ago

Codecov Report

Merging #69 into master will decrease coverage by 0.39%. The diff coverage is 100%.

@@             Coverage Diff             @@
##             master      #69     +/-   ##
===========================================
- Coverage      96.9%   96.51%   -0.4%     
+ Complexity       67       61      -6     
===========================================
  Files            17       17             
  Lines           194      172     -22     
  Branches         15        9      -6     
===========================================
- Hits            188      166     -22     
  Misses            4        4             
  Partials          2        2

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/org/cactoos/http/HtHead.java	`100% <100%> (ø)`	`3 <2> (-6)`	:arrow_down:

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 9f10348...89c7a77. Read the comment docs.

victornoel commented 5 years ago

@ilyakharlamov I've added a few comments, we are almost there :)

victornoel commented 5 years ago

@ilyakharlamov thank you for your patience, I think it's all good now :) @llorllale this is good to merge!

llorllale commented 5 years ago

@ilyakharlamov I cringed when I saw Enumeration. Is there any way we can get creative and replace enumeration with some composition of org.cactoos.io.Joined and others? It could potentially allow us not to define a class just for this.

ilyakharlamov commented 5 years ago

@llorllale That can be possible after cactoos#1035.

victornoel commented 5 years ago

@ilyakharlamov apparenty https://github.com/yegor256/cactoos/pull/1038 that solves https://github.com/yegor256/cactoos/issues/1035 has been merged a few days ago, I suppose you can continue the work here?

ilyakharlamov commented 5 years ago

@victornoel Even though the yegor256/cactoos#1038 is merged, it is not released (will be cactoos 0.39). I am not responsible for cactoos-releases, it may be a few month before we have 0.39.

victornoel commented 5 years ago

@ilyakharlamov you should ask ARC in the issue you are interested in to make a new release

victornoel commented 5 years ago

@ilyakharlamov cactoos 0.39 has been released (see https://github.com/yegor256/cactoos/issues/1035)

ilyakharlamov commented 5 years ago

@0crat wait #72

victornoel commented 5 years ago

@ilyakharlamov it has already been released... and also it is useless to tell 0crat to wait in a PR

victornoel commented 5 years ago

@ilyakharlamov just update the dependency and fix the PR

0crat commented 5 years ago

@0crat wait #72 (here)

@ilyakharlamov It's a code review job #69, can't put it on hold

ilyakharlamov commented 5 years ago

@victornoel Cactoos 0.39 introduces a few signature changes. For some reason it decided to use checked exceptions everywhere (I thought checked exceptions are dead), so it's not a minor change, it will affect dozens of files just to upgrade, so will wait for #72.

victornoel commented 5 years ago

@ilyakharlamov yes, you are right, thanks for the explanation

victornoel commented 5 years ago

@ilyakharlamov cactoos-http now uses cactoos 0.39, I believe we can continue here now :)

victornoel commented 5 years ago

@ilyakharlamov thanks for your patience, it looks real nice now :) @llorllale good to merge

llorllale commented 5 years ago

@victornoel @ilyakharlamov I just want to mention that @dgroup's hamcrest PR to add AllOf vararg ctor got merged a while back, so we should remove Matchers.allOf after their next release.

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

Code review was too long (39 days), architects (@llorllale) were penalized, see §55

0crat commented 5 years ago

The job #69 is now out of scope

0crat commented 5 years ago

Order was finished: +15 point(s) just awarded to @victornoel/z

0crat commented 5 years ago

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

yegor256 / cactoos-http

(#42) HtHead can be broken. #69

Codecov Report