You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Hindman <be...@berkeley.edu> on 2015/06/18 17:30:09 UTC
Review Request 35611: Added initial doxygen documentation for stout
flags.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35611/
-----------------------------------------------------------
Review request for mesos and Bernd Mathiske.
Repository: mesos
Description
-------
A follow up to https://reviews.apache.org/r/34943 as requested by most reviewers.
Diffs
-----
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp ee855da6128c2d671eb2fc7eaa2c0aab2341aebb
Diff: https://reviews.apache.org/r/35611/diff/
Testing
-------
make check and generated doxygen documentation.
Thanks,
Benjamin Hindman
Re: Review Request 35611: Added initial doxygen documentation for
stout flags.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35611/#review88438
-----------------------------------------------------------
Bad patch!
Reviews applied: [34943, 35611]
Failed command: ./support/apply-review.sh -n -r 35611
Error:
2015-06-18 21:24:09 URL:https://reviews.apache.org/r/35611/diff/raw/ [16549/16549] -> "35611.patch" [1]
error: patch failed: 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp:58
error: 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp: patch does not apply
Failed to apply patch
- Mesos ReviewBot
On June 18, 2015, 3:33 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35611/
> -----------------------------------------------------------
>
> (Updated June 18, 2015, 3:33 p.m.)
>
>
> Review request for mesos and Bernd Mathiske.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> A follow up to https://reviews.apache.org/r/34943 as requested by most reviewers.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp ee855da6128c2d671eb2fc7eaa2c0aab2341aebb
>
> Diff: https://reviews.apache.org/r/35611/diff/
>
>
> Testing
> -------
>
> make check and generated doxygen documentation.
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 35611: Added initial doxygen documentation for
stout flags.
Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35611/#review89362
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (line 71)
<https://reviews.apache.org/r/35611/#comment141932>
delete second "via"?
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (line 88)
<https://reviews.apache.org/r/35611/#comment141936>
Not sure what you mean with "possibly".
And "as well as"? There are no flags called PREFIX_foo and PREFIX_bar. They are simply foo and bar.
Suggestion:
"Thus the flags foo and bar can be loaded from environment variables PREFIX_foo and PREFIX_bar. Given the order of statements in this example, the values already assigned above when loading from the command line will be overwritten."
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (line 89)
<https://reviews.apache.org/r/35611/#comment141934>
typo: environment
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (line 94)
<https://reviews.apache.org/r/35611/#comment141937>
"header files"?
Did you mean "descriptions of"?
Maybe try this:
@see load()
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (line 117)
<https://reviews.apache.org/r/35611/#comment141939>
Not your bug, but this is not a grammatically correct sentence. Suggestion:
Load any flags from the environment using the variable prefix. For example, given prefix 'STOUT_' this method will load a flag named 'foo' via the environment variable 'STOUT_foo'.
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (line 119)
<https://reviews.apache.org/r/35611/#comment141940>
The latter variable should not be looked at, only the former. Env vars are case sensitive, are they not? Or if Mesos looks at both, then which one wins?
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (line 135)
<https://reviews.apache.org/r/35611/#comment141942>
Can't parse this sentence. Unclear grouping of phrases separated by conjunctions. How about:
If false return an Error once an unknown flag is encountered.
<see also the other overloads>
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (line 137)
<https://reviews.apache.org/r/35611/#comment141941>
typo: duplicates
<see also the other overloads>
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (line 138)
<https://reviews.apache.org/r/35611/#comment141943>
Suggestion:
If false return an Error once a duplicate flag is encountered.
<see also the other overloads>
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (line 183)
<https://reviews.apache.org/r/35611/#comment141945>
s/Values/Map
<see also the other overloads>
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (line 209)
<https://reviews.apache.org/r/35611/#comment141946>
We cannot point to a non-API (non-doxygenized) item using '@see' (or even 'see'). Better to explain right here what is going on.
I know this is inherited text, but best to fix it now :-)
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (line 228)
<https://reviews.apache.org/r/35611/#comment141949>
s/The/The latter
?
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (line 229)
<https://reviews.apache.org/r/35611/#comment141948>
Here, the reader will once again not understand what 'usageMessage_' is without looking at source code.
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (line 264)
<https://reviews.apache.org/r/35611/#comment141951>
s/Add's/Adds
<and also in several places below>
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (line 276)
<https://reviews.apache.org/r/35611/#comment141952>
Simpler: "This function is expected to have the signature ..."
<see also below>
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (line 279)
<https://reviews.apache.org/r/35611/#comment141953>
lambdas -> closures ?
- Bernd Mathiske
On June 20, 2015, 8:19 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35611/
> -----------------------------------------------------------
>
> (Updated June 20, 2015, 8:19 a.m.)
>
>
> Review request for mesos and Bernd Mathiske.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> A follow up to https://reviews.apache.org/r/34943 as requested by most reviewers.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp ee855da6128c2d671eb2fc7eaa2c0aab2341aebb
>
> Diff: https://reviews.apache.org/r/35611/diff/
>
>
> Testing
> -------
>
> make check and generated doxygen documentation.
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 35611: Added initial doxygen documentation for
stout flags.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35611/#review88667
-----------------------------------------------------------
Bad patch!
Reviews applied: [35611]
Failed command: ./support/apply-review.sh -n -r 35611
Error:
2015-06-20 21:26:09 URL:https://reviews.apache.org/r/35611/diff/raw/ [16549/16549] -> "35611.patch" [1]
error: patch failed: 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp:58
error: 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp: patch does not apply
Failed to apply patch
- Mesos ReviewBot
On June 20, 2015, 3:19 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35611/
> -----------------------------------------------------------
>
> (Updated June 20, 2015, 3:19 p.m.)
>
>
> Review request for mesos and Bernd Mathiske.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> A follow up to https://reviews.apache.org/r/34943 as requested by most reviewers.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp ee855da6128c2d671eb2fc7eaa2c0aab2341aebb
>
> Diff: https://reviews.apache.org/r/35611/diff/
>
>
> Testing
> -------
>
> make check and generated doxygen documentation.
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 35611: Added initial doxygen documentation for
stout flags.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35611/
-----------------------------------------------------------
(Updated June 20, 2015, 3:19 p.m.)
Review request for mesos and Bernd Mathiske.
Repository: mesos
Description
-------
A follow up to https://reviews.apache.org/r/34943 as requested by most reviewers.
Diffs
-----
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp ee855da6128c2d671eb2fc7eaa2c0aab2341aebb
Diff: https://reviews.apache.org/r/35611/diff/
Testing
-------
make check and generated doxygen documentation.
Thanks,
Benjamin Hindman
Re: Review Request 35611: Added initial doxygen documentation for
stout flags.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35611/
-----------------------------------------------------------
(Updated June 18, 2015, 3:33 p.m.)
Review request for mesos and Bernd Mathiske.
Repository: mesos
Description
-------
A follow up to https://reviews.apache.org/r/34943 as requested by most reviewers.
Diffs
-----
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp ee855da6128c2d671eb2fc7eaa2c0aab2341aebb
Diff: https://reviews.apache.org/r/35611/diff/
Testing
-------
make check and generated doxygen documentation.
Thanks,
Benjamin Hindman