You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alexander Rukletsov <ru...@gmail.com> on 2018/10/07 15:16:40 UTC
Review Request 68945: Fused constructors of `MethodNotAllowed` into
one.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68945/
-----------------------------------------------------------
Review request for mesos and Benno Evers.
Repository: mesos
Description
-------
There is no good reason to provide two c-tors for `MethodNotAllowed`,
with one taking `requestMethod` and one not. Instead, an `Option<>`
can be used. This also removes the need for copy-paste in the c-tor
body.
Diffs
-----
3rdparty/libprocess/include/process/http.hpp cef511a1563a26e170ce7f4a49de12776b4512e7
Diff: https://reviews.apache.org/r/68945/diff/1/
Testing
-------
`make check` on various linux distros
Thanks,
Alexander Rukletsov
Re: Review Request 68945: Fused constructors of `MethodNotAllowed`
into one.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68945/#review209292
-----------------------------------------------------------
Bad patch!
Reviews applied: [68945, 68944]
Failed command: python support/apply-reviews.py -n -r 68944
Error:
Traceback (most recent call last):
File "support/apply-reviews.py", line 35, in <module>
import urllib.request
ImportError: No module named request
Full log: https://builds.apache.org/job/Mesos-Reviewbot/23457/console
- Mesos Reviewbot
On Oct. 7, 2018, 3:16 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68945/
> -----------------------------------------------------------
>
> (Updated Oct. 7, 2018, 3:16 p.m.)
>
>
> Review request for mesos and Benno Evers.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> There is no good reason to provide two c-tors for `MethodNotAllowed`,
> with one taking `requestMethod` and one not. Instead, an `Option<>`
> can be used. This also removes the need for copy-paste in the c-tor
> body.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp cef511a1563a26e170ce7f4a49de12776b4512e7
>
>
> Diff: https://reviews.apache.org/r/68945/diff/1/
>
>
> Testing
> -------
>
> `make check` on various linux distros
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 68945: Fused constructors of `MethodNotAllowed`
into one.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Oct. 8, 2018, 10:49 a.m., Benno Evers wrote:
> > 3rdparty/libprocess/include/process/http.hpp
> > Lines 777 (patched)
> > <https://reviews.apache.org/r/68945/diff/2/?file=2095314#file2095314line784>
> >
> > I'd probably consider using `strings::format()` here.
It is indeed a bit nicer, but I'd say the readability benefit does not outweigh the cost of including an extra header.
```
"405 Method Not Allowed. Expecting one of { '" +
strings::join("', '", allowedMethods) + "' }" +
(requestMethod.isSome()
? ", but received '" + requestMethod.get() + "'"
: "") +
".";
```
vs.
```
strings::format(
"405 Method Not Allowed. Expecting one of { '%s' }%s.",
strings::join("', '", allowedMethods),
requestMethod.isSome()
? ", but received '" + requestMethod.get() + "'"
: "");
```
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68945/#review209305
-----------------------------------------------------------
On Oct. 8, 2018, 7:50 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68945/
> -----------------------------------------------------------
>
> (Updated Oct. 8, 2018, 7:50 a.m.)
>
>
> Review request for mesos and Benno Evers.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> There is no good reason to provide two c-tors for `MethodNotAllowed`,
> with one taking `requestMethod` and one not. Instead, an `Option<>`
> can be used. This also removes the need for copy-paste in the c-tor
> body.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp cef511a1563a26e170ce7f4a49de12776b4512e7
>
>
> Diff: https://reviews.apache.org/r/68945/diff/2/
>
>
> Testing
> -------
>
> `make check` on various linux distros
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 68945: Fused constructors of `MethodNotAllowed`
into one.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68945/#review209305
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/include/process/http.hpp
Lines 777 (patched)
<https://reviews.apache.org/r/68945/#comment293619>
I'd probably consider using `strings::format()` here.
- Benno Evers
On Oct. 8, 2018, 7:50 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68945/
> -----------------------------------------------------------
>
> (Updated Oct. 8, 2018, 7:50 a.m.)
>
>
> Review request for mesos and Benno Evers.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> There is no good reason to provide two c-tors for `MethodNotAllowed`,
> with one taking `requestMethod` and one not. Instead, an `Option<>`
> can be used. This also removes the need for copy-paste in the c-tor
> body.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp cef511a1563a26e170ce7f4a49de12776b4512e7
>
>
> Diff: https://reviews.apache.org/r/68945/diff/2/
>
>
> Testing
> -------
>
> `make check` on various linux distros
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 68945: Fused constructors of `MethodNotAllowed`
into one.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68945/#review209302
-----------------------------------------------------------
PASS: Mesos patch 68945 was successfully built and tested.
Reviews applied: `['68944', '68945']`
All the build artifacts available at: http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2433/mesos-review-68945
- Mesos Reviewbot Windows
On Oct. 8, 2018, 9:50 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68945/
> -----------------------------------------------------------
>
> (Updated Oct. 8, 2018, 9:50 a.m.)
>
>
> Review request for mesos and Benno Evers.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> There is no good reason to provide two c-tors for `MethodNotAllowed`,
> with one taking `requestMethod` and one not. Instead, an `Option<>`
> can be used. This also removes the need for copy-paste in the c-tor
> body.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp cef511a1563a26e170ce7f4a49de12776b4512e7
>
>
> Diff: https://reviews.apache.org/r/68945/diff/2/
>
>
> Testing
> -------
>
> `make check` on various linux distros
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 68945: Fused constructors of `MethodNotAllowed`
into one.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68945/
-----------------------------------------------------------
(Updated Oct. 8, 2018, 7:50 a.m.)
Review request for mesos and Benno Evers.
Repository: mesos
Description
-------
There is no good reason to provide two c-tors for `MethodNotAllowed`,
with one taking `requestMethod` and one not. Instead, an `Option<>`
can be used. This also removes the need for copy-paste in the c-tor
body.
Diffs (updated)
-----
3rdparty/libprocess/include/process/http.hpp cef511a1563a26e170ce7f4a49de12776b4512e7
Diff: https://reviews.apache.org/r/68945/diff/2/
Changes: https://reviews.apache.org/r/68945/diff/1-2/
Testing
-------
`make check` on various linux distros
Thanks,
Alexander Rukletsov
Re: Review Request 68945: Fused constructors of `MethodNotAllowed`
into one.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68945/#review209293
-----------------------------------------------------------
FAIL: Some of the unit tests failed. Please check the relevant logs.
Reviews applied: `['68944', '68945']`
Failed command: `Start-MesosCITesting`
All the build artifacts available at: http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2431/mesos-review-68945
Relevant logs:
- [mesos-tests-cmake.log](http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2431/mesos-review-68945/logs/mesos-tests-cmake.log):
```
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\mt_adaptor.c(496): warning C4244: '=': conversion from 'time_t' to 'int32_t', possible loss of data [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(256): warning C4090: 'function': different 'const' qualifiers [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(166): warning C4716: 'pthread_cond_broadcast': must return a value [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(205): warning C4716: 'pthread_cond_wait': must return a value [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(124): warning C4996: 'fopen': This function or variable may be unsafe. Consider using fopen_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details. [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(128): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(279): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(301): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(368): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(372): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(512): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(543): warning C4996: 'strcpy': This function or variable may be unsafe. Consider using strcpy_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details. [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(548): warning C4996: 'fopen': This function or variable may be unsafe. Consider using fopen_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details. [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(569): warning C4996: 'strcpy': This function or variable may be unsafe. Consider using strcpy_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details. [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
"D:\DCOS\mesos\src\tests\mesos-tests.vcxproj" (default target) (1) ->
(ClCompile target) ->
d:\dcos\mesos\mesos\src\tests\executor_http_api_tests.cpp(356): error C2440: '<function-style-cast>': cannot convert from 'initializer list' to 'process::http::MethodNotAllowed' [D:\DCOS\mesos\src\tests\mesos-tests.vcxproj]
d:\dcos\mesos\mesos\src\tests\executor_http_api_tests.cpp(356): error C2660: 'AwaitAssertResponseStatusEq': function does not take 5 arguments [D:\DCOS\mesos\src\tests\mesos-tests.vcxproj]
d:\dcos\mesos\mesos\src\tests\executor_http_api_tests.cpp(356): error C2512: 'testing::AssertionResult': no appropriate default constructor available [D:\DCOS\mesos\src\tests\mesos-tests.vcxproj]
d:\dcos\mesos\mesos\src\tests\scheduler_http_api_tests.cpp(853): error C2440: '<function-style-cast>': cannot convert from 'initializer list' to 'process::http::MethodNotAllowed' [D:\DCOS\mesos\src\tests\mesos-tests.vcxproj]
d:\dcos\mesos\mesos\src\tests\scheduler_http_api_tests.cpp(853): error C2660: 'AwaitAssertResponseStatusEq': function does not take 5 arguments [D:\DCOS\mesos\src\tests\mesos-tests.vcxproj]
d:\dcos\mesos\mesos\src\tests\scheduler_http_api_tests.cpp(853): error C2512: 'testing::AssertionResult': no appropriate default constructor available [D:\DCOS\mesos\src\tests\mesos-tests.vcxproj]
172 Warning(s)
6 Error(s)
Time Elapsed 00:29:16.69
```
- Mesos Reviewbot Windows
On Oct. 7, 2018, 3:16 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68945/
> -----------------------------------------------------------
>
> (Updated Oct. 7, 2018, 3:16 p.m.)
>
>
> Review request for mesos and Benno Evers.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> There is no good reason to provide two c-tors for `MethodNotAllowed`,
> with one taking `requestMethod` and one not. Instead, an `Option<>`
> can be used. This also removes the need for copy-paste in the c-tor
> body.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp cef511a1563a26e170ce7f4a49de12776b4512e7
>
>
> Diff: https://reviews.apache.org/r/68945/diff/1/
>
>
> Testing
> -------
>
> `make check` on various linux distros
>
>
> Thanks,
>
> Alexander Rukletsov
>
>