#306: Modify BkBasic for persistent conections according to HTTP 1.1 #337
Conversation
|
@lautarobock Thanks, I will find someone to review this PR soon |
|
@pinaf please review,thanks |
| * Create a joiner for a header. | ||
| * @return Joiner | ||
| */ | ||
| private Joiner joiner() { |
There was a problem hiding this comment.
@lautarobock instead of extracting this into a method, you should create a private static final String with the \r\n string.
|
@lautarobock sorry for the delay |
|
@lautarobock 12 comments above |
|
@lautarobock nice PR! |
|
@pinaf there is a new commit with fixes |
|
@lautarobock looks good |
|
@lautarobock thanks |
|
@rultor merge |
|
@lautarobock the implementation is OK, but the way you test it is rather bad. Mocking, in general, is a bad practice, see http://www.yegor256.com/2014/09/23/built-in-fake-objects.html Instead of mocking a |
|
@yegor256 ok, but the test with mocks was there before. I will change the test, but I think more for a |
|
@yegor256 Is there a way to make |
|
@yegor256 Is this what you were looking for? |
|
@pinaf @lautarobock I don't really like this boolean flag "persistent" in the class. It would be much better to create another class, something like |
|
@lautarobock yes, we should somehow get rid of code duplication. let's think... |
|
@yegor256 IMHO I think the actual solution in the minimal enough, the best is to create a new bug with this refactor. |
|
@lautarobock current solution makes the code hard to read. I would recommend you to keep the unit test in the branch and remove the implementation. Then, add a puzzle to the test. Someone later will implement it. |
|
@yegor256 done, remove implementation and let test |
|
@lautarobock there are checkstyle violations. please fix them and get the travis build to go green. |
|
@pinaf new commit with this fixes, sorry |
|
@lautarobock I don't see the puzzle @yegor256 asked for |
|
@pinaf yes, but, what this mind in this case? |
|
@lautarobock sorry, I don't understand |
|
@lautarobock hello? |
|
@lautarobock if you are not going to merge, could you please close this PR? Thanks! |
|
@lautarobock ping |
|
@pinaf done |
|
@elenavolokhova please, let us know what do you think about this ticket, according to our QA rules |
According to our quality rules:
As far as you are the performer of this ticket you are responsible for its quality. Please make sure that everyone follows this rule in your tickets in the future. Ask them to fix issues before closing the ticket. Please confirm that everything is clear and you have no questions regarding this approach. |
|
@elenavolokhova sorry. I'll pay more attention. |
|
@pinaf Thank you! |
|
@davvd Quality is acceptable here. |
@elenavolokhova thanks for the QA review, we'll work better next time |
|
@pinaf I added 10 mins to @elenavolokhova (for QA review) in transaction 61660897... 29 mins was added to your account, many thanks for your contribution (payment AP-3CV882267J514640X)! The task took 1006 hours and 59 mins.... extra minutes for review comments (c=14)... +29 added to your rating, at the moment it is: +5729 |
#306 Modify BkBasic checking headers and modifying resposes for http conections according to HTTP 1.1