> On June 5, 2018, 10:46 a.m., Alexander Rukletsov wrote: > > 3rdparty/libprocess/include/process/http.hpp > > Line 701 (original), 701-702 (patched) > > <https://reviews.apache.org/r/67414/diff/1/?file=2034492#file2034492line701> > > > > Why not using `process::http::Status::string()` > > https://github.com/apache/mesos/blob/2198b961d24b788564d36490cf52f78d7ec07655/3rdparty/libprocess/src/http.cpp#L176-L180 > > instead? > > > > Maybe then you can even put a single line into `Response` c-tor instead > > of modifying individual classes.
I didn't know about this function, but looking at it it seems to be in direct violation of our style guide at (https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables) so we should probably look into deprecating it rather than promoting further usage. But even w/o that issue, I'm not sure that ``` BadRequest() : Response(Status::string(Status::BAD_REQUEST), Status::BAD_REQUEST) {} ``` is better than ``` BadRequest() : Response("400 Bad Request.", Status::BAD_REQUEST) {} ``` I dont think putting it in the default constructor of `Response` is a good idea, because we more or less endorse constructing responses like this ``` process::http::OK response; response.type = response.PATH; response.path = path; response.headers["Content-Type"] = "application/octet-stream"; response.headers["Content-Disposition"] = strings::format("attachment; filename=%s", path).get(); ``` and we would need to audit all usages of such responses to ensure they're not confused by suddenly having an additional body. - Benno ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67414/#review204313 ----------------------------------------------------------- On June 1, 2018, 3:14 p.m., Benno Evers wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67414/ > ----------------------------------------------------------- > > (Updated June 1, 2018, 3:14 p.m.) > > > Review request for mesos. > > > Repository: mesos > > > Description > ------- > > By default on error libprocess would only return a response > with the correct status code and no response body. > > However, most browsers do not visually indicate the response > status code, making it hard for the user to figure out what > exactly the problem was. > > > Diffs > ----- > > 3rdparty/libprocess/include/process/http.hpp > 055447e13117c0a3ba79d0fc326ece657e8f064f > > > Diff: https://reviews.apache.org/r/67414/diff/1/ > > > Testing > ------- > > > Thanks, > > Benno Evers > >
