mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Benjamin Hindman" <b...@berkeley.edu>
Subject Re: Review Request 16839: Incorporate CR feedback.
Date Wed, 15 Jan 2014 02:49:49 GMT

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

Ship it!


This is great Charlie! Mostly nits and nats on style. I'll get this committed after you update.


3rdparty/libprocess/src/process.cpp
<https://reviews.apache.org/r/16839/#comment60597>

    s/if(/if (/
    
    Also, can we put a newline before the 'if' please?



3rdparty/libprocess/src/process.cpp
<https://reviews.apache.org/r/16839/#comment60598>

    I'd love to see a TODO which let's us specify the content type as a 6th parameter. In
the mean time, can you leave a comment as to why you picked application/x-ww-form-urlencoded
as the default?



3rdparty/libprocess/src/process.cpp
<https://reviews.apache.org/r/16839/#comment60612>

    Yes, let's have the interface take an 'const Option<String>& query'. You can
use Some("constant string") when the compiler gives you crap for trying to go from a const
char * to an Option<string>.



3rdparty/libprocess/src/process.cpp
<https://reviews.apache.org/r/16839/#comment60599>

    Agreed with Jie, let's take an optional 'query'. Also, for future use, you can just use
'None()' instead of 'Option<string>::none()'.



3rdparty/libprocess/src/tests/http_tests.cpp
<https://reviews.apache.org/r/16839/#comment60600>

    Please remove the whitespace around the parameter.



3rdparty/libprocess/src/tests/http_tests.cpp
<https://reviews.apache.org/r/16839/#comment60611>

    EXPECT_TRUE?



3rdparty/libprocess/src/tests/http_tests.cpp
<https://reviews.apache.org/r/16839/#comment60601>

    Please remove the whitespace around the parameter.



3rdparty/libprocess/src/tests/http_tests.cpp
<https://reviews.apache.org/r/16839/#comment60610>

    Kill spaces around '_' please.



3rdparty/libprocess/src/tests/http_tests.cpp
<https://reviews.apache.org/r/16839/#comment60609>

    Kill spaces around '_' please.



3rdparty/libprocess/src/tests/http_tests.cpp
<https://reviews.apache.org/r/16839/#comment60602>

    Please wrap after the '='.



3rdparty/libprocess/src/tests/http_tests.cpp
<https://reviews.apache.org/r/16839/#comment60603>

    Two newlines between top-level declarations please.



3rdparty/libprocess/src/tests/http_tests.cpp
<https://reviews.apache.org/r/16839/#comment60607>

    Kill spaces around parameter please.



3rdparty/libprocess/src/tests/http_tests.cpp
<https://reviews.apache.org/r/16839/#comment60608>

    EXPECT_TRUE?



3rdparty/libprocess/src/tests/http_tests.cpp
<https://reviews.apache.org/r/16839/#comment60604>

    Another newline please.



3rdparty/libprocess/src/tests/http_tests.cpp
<https://reviews.apache.org/r/16839/#comment60606>

    Kill spaces around '_' please.



3rdparty/libprocess/src/tests/http_tests.cpp
<https://reviews.apache.org/r/16839/#comment60605>

    Wrap after '=' please.


- Benjamin Hindman


On Jan. 15, 2014, 2:05 a.m., Charlie Carson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16839/
> -----------------------------------------------------------
> 
> (Updated Jan. 15, 2014, 2:05 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, Jeff Currier, and Jie Yu.
> 
> 
> Bugs: https://issues.apache.org/jira/browse/MESOS-902
>     https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-902
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
>    Add post function to http.hpp of libprocess.
> 
>     This adds the post equivalent of the get function to http.hpp.
> 
>     The existing get method is refactored into internal::httpRequest
>     with arguments for method, query, and body.
> 
>     The get and post functions can then be implemented as simple
>     wrapper of internal::httpRequest.
> 
>     There are also new unit tests to verify the existing behavior of
>     get and the new post behavior.
> 
>     See:  https://issues.apache.org/jira/browse/MESOS-902
> 
>     Review: https://reviews.apache.org/r/16839
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/http.hpp 5bdd520c9e0bcc0a2d3a4554cc4ced99dcf78b51

>   3rdparty/libprocess/src/process.cpp 67f7f9b3b05c8bc9b0f6281689223996ddfa68d1 
>   3rdparty/libprocess/src/tests/http_tests.cpp 68d1a1b2ed5645d861b1e613aed9731368ebdc6a

> 
> Diff: https://reviews.apache.org/r/16839/diff/
> 
> 
> Testing
> -------
> 
> added new unit tests
> make check
> 
> 
> Thanks,
> 
> Charlie Carson
> 
>


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