-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36402/#review94105
-----------------------------------------------------------


Looking better! Can you please pull out the fixes to 'acceptsEncoding' into a 
separate review? Also, would like to see tests for the cases that weren't 
handled correctly (e.g. "gzipp;q=1.0" should not match "gzip"). Pulling the 
changes apart helps avoid extra round-trips on reviews (see here: 
http://mesos.apache.org/documentation/latest/effective-code-reviewing/) :)


3rdparty/libprocess/include/process/http.hpp (lines 119 - 126)
<https://reviews.apache.org/r/36402/#comment148611>

    Let's keep the RFC reference for the implementation, mind moving it back?
    
    Also, please remove my TODO for now, we can see how Request evolves over 
time.



3rdparty/libprocess/src/http.cpp (line 146)
<https://reviews.apache.org/r/36402/#comment148609>

    Why the change here? Should still say 'encoding' right?



3rdparty/libprocess/src/http.cpp (line 155)
<https://reviews.apache.org/r/36402/#comment148607>

    This will crash the program if tokens is empty!



3rdparty/libprocess/src/http.cpp (line 156)
<https://reviews.apache.org/r/36402/#comment148620>

    The style checker doesn't like this



3rdparty/libprocess/src/http.cpp (lines 157 - 159)
<https://reviews.apache.org/r/36402/#comment148610>

    We don't need to use pairs anymore, right? Looks like we should just call 
tokenize again on 'tokens[1]' (if present).



3rdparty/libprocess/src/http.cpp (line 218)
<https://reviews.apache.org/r/36402/#comment148608>

    This will crash the program if tokens is empty!



3rdparty/libprocess/src/http.cpp (lines 219 - 221)
<https://reviews.apache.org/r/36402/#comment148612>

    Ditto here, can we tokenize 'tokens[1]' instead of using strings::pairs?



3rdparty/libprocess/src/http.cpp (line 235)
<https://reviews.apache.org/r/36402/#comment148619>

    newline here?



3rdparty/libprocess/src/tests/http_tests.cpp (line 693)
<https://reviews.apache.org/r/36402/#comment148613>

    Mind moving this above with the other http tests?



3rdparty/libprocess/src/tests/http_tests.cpp (lines 696 - 703)
<https://reviews.apache.org/r/36402/#comment148614>

    We don't generally wrap things this far, can you just do the following?
    
    ```
     vector<string> wrongHeaders = {
         "test,q=0.0",
         ...
     };
    ```
    
    Ditto below.



3rdparty/libprocess/src/tests/http_tests.cpp (line 706)
<https://reviews.apache.org/r/36402/#comment148615>

    'auto' here isn't buying us much over just 'const string&', we try to use 
'auto' where it helps readability
    
    Also please use foreach for now



3rdparty/libprocess/src/tests/http_tests.cpp (lines 709 - 710)
<https://reviews.apache.org/r/36402/#comment148618>

    Ditto here, can you just print 'accept'?



3rdparty/libprocess/src/tests/http_tests.cpp (line 727)
<https://reviews.apache.org/r/36402/#comment148616>

    Ditto here, 'auto' doesn't seem very helpful to readability here.
    
    Also, please use foreach for now.



3rdparty/libprocess/src/tests/http_tests.cpp (lines 729 - 731)
<https://reviews.apache.org/r/36402/#comment148617>

    Can you just print out 'accept' rather than grabbing the header and calling 
.get()?


- Ben Mahler


On Aug. 4, 2015, 5:42 p.m., Isabel Jimenez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> -----------------------------------------------------------
> 
> (Updated Aug. 4, 2015, 5:42 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and 
> Vinod Kone.
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> -------
> 
> Adding a method for Accept header in request + refactor of Accept-Encoding
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/http.hpp b8d9300 
>   3rdparty/libprocess/src/http.cpp 4dcbd74 
>   3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd5 
> 
> Diff: https://reviews.apache.org/r/36402/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Reply via email to