mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ben Mahler" <benjamin.mah...@gmail.com>
Subject Re: Review Request 36402: Adding 'Accept' header in request
Date Tue, 11 Aug 2015 19:55:44 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message