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

Ship it!


Mostly some minor comments, I'll make the adjustments and land this for you!


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

    Should we do this the same way in both acceptsEncoding and 
acceptsMediaType? Note that trim only strips from the front and back, so we 
should be stripping after we've tokenized on ';'. Also, strings::pairs will 
result in keys that contain whitespace as well with this approach.
    
    I'd suggest we keep the whitespace removal the same as acceptsEncoding for 
now.



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

    Might be nice to avoid the implicit relyance on tokens being non-empty here 
as well.



3rdparty/libprocess/src/tests/http_tests.cpp (lines 661 - 669)
<https://reviews.apache.org/r/36402/#comment149718>

    These should all use ; to delimit the q value rather than ,



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

    Let's put this in the loop where it's used?



3rdparty/libprocess/src/tests/http_tests.cpp (lines 675 - 677)
<https://reviews.apache.org/r/36402/#comment149717>

    How about:
    
    ```
    EXPECT_FALSE(request.acceptsMediaType("test/*"))
        << "Not expecting '" << accept << "' to match 'test/*'";
    ```



3rdparty/libprocess/src/tests/http_tests.cpp (lines 682 - 693)
<https://reviews.apache.org/r/36402/#comment149720>

    Ditto here, q values are delimited by semi colons, it looks like this test 
treats q values as possible accept types.



3rdparty/libprocess/src/tests/http_tests.cpp (lines 698 - 700)
<https://reviews.apache.org/r/36402/#comment149723>

    How about:
    
    ```
    EXPECT_TRUE(request.acceptsMediaType("text/html"))
        << "Expecting '" << accept << "' to match 'text/html'";
    ```


- Ben Mahler


On Aug. 10, 2015, 9:52 p.m., Isabel Jimenez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> -----------------------------------------------------------
> 
> (Updated Aug. 10, 2015, 9:52 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